ChuanqiXu planned changes to this revision. ChuanqiXu marked an inline comment as done. ChuanqiXu added a comment.
In D130864#3693019 <https://reviews.llvm.org/D130864#3693019>, @ilya-biryukov wrote: > 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. No, we don't know it is a bottleneck in practice. (Currently there shouldn't be many users for C++20 Modules). The reason to do this is that we (Iain and me) feel bad to compare string frequently. Benchmarks would be good but I am afraid I can't. Since there are not enough existing benchmarks and it looks costful to construct the meaningful benchmarks. Let's not make the perfect to be the enemy of better. > - `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. Good idea. Will try to do. 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
