ilya-biryukov added a comment. Thanks for working on this. I have a few major concerns here:
- Do we know that it's a bottleneck in practice? E.g. if the module name have different sizes, comparing strings is actually fast. If the module names are short, we are also in a pretty good situation. Having some benchmarks numbers would help to quantify whether the change is needed. - `llvm::hash_value` is not a crypto hash, so we should expect collisions from it. OTOH, doing an extra string comparison when hashes match defeats the purpose of this optimization. Collisions are rare, but if someone hits one after the compiler is released there are literally no workarounds. Instead we can use `llvm::SHA1` or `llvm::SHA256`. (possibly truncated to 128 bits? it's hard to tell how much this is needed without benchmarks.) - Why not store the hash directly inside `Module` and compute it on construction after setting name? If we want these comparisons to be fast, we probably want to avoid even the overhead of hash table access. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130864/new/ https://reviews.llvm.org/D130864 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
