[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

> I've seen quite nice benchmarks for kernel build times in the issues you've mentioned it would be nice if you could back that claim with similar data(not that I don't take your word for it, I just love tables and I am jealous that kernel has some but we don't :( )

Heh, those were painstakingly written in markdown by hand, using `CC="/usr/bin/time -v clang"` to test.  The weren't statistically significant (N=1), nor were they autogenerated.  My claims about builds of LLVM are similarly non-scientific in terms of statistical significance.

> I can say this patch LG from my side, but my understanding of this code isn't quite strong so I would wait for a couple more days to see if someone else will chime in.
>  Also forgot to say, thanks a lot for taking your time to investigate this issue and coming up with a patch!

That's ok, I think generally finding someone to take the time to review pieces of a large project is generally difficult, and that contributors should feel empowered to be able to sign off on modifications to code they themselves did not write.  Thank you for taking the time to review it!

I will break this up into 3 patches, and ask you kindly to review once I have them ready.  I will post them here, too, in case other reviewers or subscribers would like to follow along, then I'll Abandon this revision.



================
Comment at: clang/include/clang/Basic/SourceManager.h:1747
       return getLoadedSLocEntryByID(ID, Invalid);
-    return getLocalSLocEntry(static_cast<unsigned>(ID), Invalid);
+    return getLocalSLocEntry(static_cast<unsigned>(ID));
   }
----------------
kadircet wrote:
> i think this deserves its separate patch.
> 
> We still can satisfy the interface requirements of libclang by introducing another member `getLocalSLocEntryWithInvalid` and change the assertion into an if check, populating `Invalid` in case of failure, what to return is more tricky though it is a const ref, I suppose we can return a dummy entry.
> 
> Feel free to keep the newly added calls without an `Invalid` argument, but I wouldn't modify the already existing ones in this patch.
> the interface requirements of libclang 

Oh? I must have missed that. Can you tell me more about that interface?  I would have thought this would fail to build if I messed up an interface?


================
Comment at: clang/lib/Basic/SourceManager.cpp:896
+    FileID Res = FileID::get(MiddleIndex);
+    if (isOffsetInLocalFileID(Res, SLocOffset)) {
+      // Remember it.  We have good locality across FileID lookups.
----------------
kadircet wrote:
> we already call getLocalSLocEntry for `MiddleIndex` above, moreover we even early exit `if (SlocOffset < MidOffset)` which is the second case in your new function. So I would say we could even gain more by:
> 
> ```
> const auto &SLocForMiddle = getLocalSLocEntry(MiddleIndex);
> auto MidOffset = SLocForMiddle.getOffset();
> .
> .
> .
> if (MiddleIndex + 1 ==LocalSLocEntryTable.size() || SLocOffset < getLocalSLocEntry(MiddleIndex + 1).getOffset()) {
> // we've got a hit!
> }
> ```
> 
> That way you also wouldn't need to introduce a new function.
> 
> Even though there's value in new helpers, IMO `SourceManager` is already complicated and has enough variants of each function to shoot yourself in the foot. If you want feel free to put a FIXME saying `we can extract a isOffsetInLocalFile helper from these lines if usage is more spread`.
> 
> 
> Again I would say this is worth doing in a separate patch. As it should be a clear win for all.
Ah, right! This is a great suggestion! And it looks correct to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80681