[PATCH] D153692: Fixed D147740 - [llvm-profdata] Refactoring Sample Profile Reader to increase FDO build speed using MD5 as key to Sample Profile map

[William Junda Huang via Phabricator via llvm-commits]

huangjd added a comment.

> I don't think there's much we can do still, even if there's collision report. Maybe we can revert back to use full name instead of MD5, if that turns out to be a problem. But as you mentioned we already support MD5 in profile generation.

If a collision is rare while there is a strong demand for correctness by users, we can add some ad-hoc logic to deal with that, which is still faster than using a multimap, and much faster than using the full function name. If collision happens frequently, then we have no choice but to revert back to use full name, but this is extremely unlikely. I couldn't find any research on partial MD5 (we only use 64-bit of it) collision using ASCII strings, so that's why I am not ruling out this out of caution.

> Can you elaborate? Is that critical or just for convenience?

It's to enforce OOP practice. The optimization passes see SampleProfileMap, which is supposed to map Functions to SampleProfiles. It should not care how keys are actually represented, and it's much more cleaner to write profiles.find(FuncName) than profiles.find(MD5Hash(FuncName)). The latter has another potential risk of misuse, because there already exist multiple ways to get a string's hash value: MD5Hash, llvm::hash_value, std::hash, and they are all different, we have to make sure the correct hash function is used at all times.

> FWIW, for CallTargetMap, we experimented with changing from StringMap (which is bad since it owns copies of strings) to DenseMap<StringRef, uint64_t>, and it regressed memory usage noticeably. The problem is DenseMap grows to 64 entries on first insertion, but CallTargetMap is usually very sparse with zero or a few entries. FunctionSamplesMap is likely sparse too.

Yes, I am aware of that. I am planning to change it to HashKeyMap<std::map, StringRef, uint64_t> as well (or unordered_map, depends on which one is more beneficial. After that I also have a plan to use specialized data structure for CallTargetMap and FunctionSamplesMap because it's true that they have zero or one entries for almost all the cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153692/new/

https://reviews.llvm.org/D153692