[PATCH] D130864: [NFC] Introduce ASTContext::isInSameModule()

[Iain Sandoe via Phabricator via cfe-commits]

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