iains added a comment. In D130864#3693022 <https://reviews.llvm.org/D130864#3693022>, @ChuanqiXu wrote:
> 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. We do not have benchmarks, it is true - initially, it seemed that the problem would be confined to the case of checking for the parent of a module partition which would be no concern - since it is one check per TU). However, in later parts of the implementation it has become necessary to check when merging or comparing individual decls - and that is a concern. >> - `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. +1. if the string is stored locally to the Module, then would it not be sufficient to compare the address? 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
