search

xd009642 / llvm-profparser

Mostly complete pure rust implementation of parsing llvm instrumentation profile data
Apache License 2.0
13 stars 9 forks source link

Cache the name md5 hash inside NamedInstrProfRecord #29

Closed Luthaf closed 1 year ago

Luthaf commented 1 year ago

Not recomputing it everytime brings CoverageMapping::generate_report from 30s per test to 1s per test on my system when used inside cargo tarpaulin.

I realised the Mapping coverage data to source part of cargo tarpaulin was very slow for me, and CoverageMapping::generate_report was taking most of it. The flamegraph showed that most of that time was spend recomputing md5 hash in get_simple_counters. Computing the hash only once when creating the NamedInstrProfRecord dramatically cuts the run time.

There might be more gains to have by changing the data structure from a Vec<NamedInstrProfRecord> to a HashMap<(u64, u64), NamedInstrProfRecord> (using the fn_hash and name_hash as keys), but I'm not sure of the greater implications of this.

xd009642 commented 1 year ago

Awesome, I'll take a look when I'm done with work for the day (and when it's merged might fast track a new llvm profparsers release and tarpaulin release to make use of it :pray: )

Luthaf commented 1 year ago

I ran cargo fmt and force pushed!

xd009642 commented 1 year ago

All looks good to me (CI failure is me needing to rejiggle my actions to be more external contributor friendly RE coverage).

With the Vec -> HashMap change, I think it may have implications in the ordering functions are printed out by the profparser binary which I use to compare to the LLVM implementation for correctness in some tests. But it may not, need to either work out from looking at the code if it'll change report printout or just implement it and see

Luthaf commented 1 year ago

Ok, I might play with Vec => HashMap next time tarpaulin feels too slow and I have time to work on it 😃 Thanks for merging!