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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
