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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I am not the best person to review this, but dig a little bit into your issues and history of this file. Your diagnosis and fix seems reasonable, AFAICT the existing caching behavior was chosen to optimize files with many macro invocations that are expanding into relatively few tokens. So this case will definitely regress after this patch, proportional to `number_of_macro_name_tokens/number_of_total_tokens` but I would expect that ratio to be quite small in any sane file hence regression to be negligible.

OTOH, when that ratio is low, if number of tokens coming from each/some macro expansions is huge (which IIUC is your case) updating that cache makes a lot of sense.

Also your claim on `builds of LLVM itself don't change due to this patch.` sounds promising. 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 :( )

Apart from all of this, I believe this patch (even though has relatively few lines) contains 3 distinct patches that should be split:

- Changing caching behaviour to apply to macro expansions
- Adding a more optimized way of checking if an offset is inside a local file
- Getting rid of some "dead" code.

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.



================
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));
   }
----------------
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.


================
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.
----------------
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.


================
Comment at: clang/lib/Basic/SourceManager.cpp:967
 
     if (isOffsetInFileID(FileID::get(-int(MiddleIndex) - 2), SLocOffset)) {
       FileID Res = FileID::get(-int(MiddleIndex) - 2);
----------------
my comments above for `isOffsetnLocalFileID` applies to here as well, in case you decide to move them into a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80681