Care to critique this code for performance?

Performance is not what I'd hoped. Am I making any glaring mistakes? Thanks.

#[derive(Serialize, Deserialize, Debug, Eq, Hash, PartialEq, Clone)]
pub struct PositionKey {
    pub symbol: String,
    pub group: String,
    pub fund: String,
    pub broker: String,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct Position {
    pub key: PositionKey,
    pub country: String,
    pub qty: f64,
}

pub struct Holdings {
    pub groups: Vec<String>,
    pub symbols: Vec<String>,
    pub countries: Vec<String>,
    pub positions: HashMap<PositionKey, Position>,
}

impl Holdings {
    pub fn new() -> Self {
        let now = Instant::now();
        let mut positions: HashMap<PositionKey, Position> = HashMap::new();
        let symbols: Vec<String> = (0..1000).map(|i| i.to_string()).collect();
        let groups: Vec<String> = ('A'..'Z').map(|i| i.to_string()).collect();
        let countries: Vec<String> = vec!["US", "CAN", "UK", "FR", "DE", "JP", "CH", "AU", "NZ"]
            .iter()
            .map(|i| i.to_string())
            .collect();
        let mut rng = rand::thread_rng();

        for _ in 1..40000 {
            let symbol_index = rng.gen_range(0..symbols.len());
            let group_index = rng.gen_range(0..groups.len());
            let country_index = rng.gen_range(0..countries.len());

            let key = PositionKey {
                symbol: symbols[symbol_index].clone(),
                group: groups[group_index].clone(),
                fund: "F1".to_string(),
                broker: "B1".to_string(),
            };

            let existing = positions.entry(key.clone());
            match existing {
                Entry::Occupied(o) => {
                    let mut p = o.into_mut();
                    p.qty += 75.38;
                }
                Entry::Vacant(v) => {
                    let position = Position {
                        key: key,
                        country: countries[country_index].clone(),
                        qty: 100.0,
                    };
                    v.insert(position);
                }
            }
        }

        println!("Holdings construction time: {:?}", now.elapsed());

        Self {
            groups,
            symbols,
            countries,
            positions: positions,
        }
    }
}

pub fn run_exposure_calcs(holdings: &Holdings) -> Vec<f64> {
    let now = Instant::now();
    let prices = get_prices(holdings);
    let positions = holdings.positions.values().collect::<Vec<&Position>>();
    let mvs: HashMap<PositionKey, f64> = calc_mv(&positions, &prices);
    //let by_symbol = holdings.positions.iter().filter(|k}|k.symbol).collect::<Vec<&Position>>();
    let mut result = Vec::new();

    let mut mv1: f64 = 0.0;
    for symbol in &holdings.symbols {
        mv1 += collect_mv(&positions, &mvs, |p| &p.key.symbol, &symbol);
    }
    result.push(mv1);
    println!("Grouped MV by {} symbols", holdings.symbols.len());

    let mut mv2: f64 = 0.0;
    for group in &holdings.groups {
        mv2 += collect_mv(&positions, &mvs, |p| &p.key.group, &group);
    }
    result.push(mv2);
    println!("Grouped MV by {} groups", holdings.groups.len());

    println!("Compliance time: {:?}", now.elapsed());

    result
}

fn collect_mv<F>(
    positions: &Vec<&Position>,
    mvs: &HashMap<PositionKey, f64>,
    accessor: F,
    group_by: &str,
) -> f64
where
    F: Fn(&Position) -> &str,
{
    let filtered = positions.into_iter().filter(|p| accessor(p) == group_by);
    let mut result: f64 = 0.0;
    for position in filtered {
        let mv = mvs.get(&position.key).unwrap();
        result += mv
    }
    result
}

fn calc_mv(positions: &Vec<&Position>, prices: &HashMap<String, f64>) -> HashMap<PositionKey, f64> {
    let mut result = HashMap::new();
    let mut calcs = 0;
    for position in positions {
        let key: &PositionKey = &position.key;
        let price = *prices.get(&key.symbol).unwrap();
        calcs += 1;
        result.insert(key.clone(), calc(position, price));
    }
    println!("Total MV Calcs: {}", calcs);
    result
}

fn get_prices(holdings: &Holdings) -> HashMap<String, f64> {
    let mut rng = rand::thread_rng();
    let mut prices = HashMap::new();
    for symbol in holdings.symbols.iter() {
        let px = rng.gen_range(0..10000);
        prices.insert(symbol.clone(), px as f64);
    }
    prices
}

fn calc(position: &Position, price: f64) -> f64 {
    position.qty * price
}

That's a lot of code without context. What is this supposed to do? What is the runtime/asymptotic complexity you anticipated and what you have measured instead? Are you running the code in release mode?

Have you compiled with --release?

Also, try to avoid String::clone or to_string in loops when it's possible to use a &str. (one main struct could own the Strings and live longer than all the others who just own references)

And I think you can run cargo clippy.

1 Like

Try usingFxHashMap instead of HashMap.

1 Like

You could try using type-specific enums instead of String, e.g. countries: Vec<String> could be countries: Vec<Country>, which could be a byte or two instead of a heap-allocated string.

3 Likes

Release build made it ~7x faster. Thanks.

It would significantly complicate the code the avoid most of the String clones - the copies are used as keys in new collections, which should own them. Yes, I see, I could use refs instead, but - it would complicate things.

That may be Ok, but, this is a feasibility PoC, and code complexity is a factor in language selection also.

At least now it's almost 2x faster than the Kotlin version.

An easy change that might speed things up:

type Country2Code = [u8; 2];

and then

        let countries: Vec<Country2Code> = [
            b"US", b"CA", b"UK", b"FR", b"DE", b"JP", b"CH", b"AU", b"NZ",
        ]
        .into_iter()
        .copied()
        .collect();

Which comes with the nice bonus of fixing the "you used an ISO 3-code for Canada but ISO 2-codes for the rest" problem.

Then a couple of other simple updates to align they types in things like Position: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ba830d6f66b8650871a0a4619ba65c5f.

(This is basically the "use an enum for countries" idea, but simplified to not need a list of them all.)

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.