Skip to content

[clang] Optimize SourceManager.getSpellingLocSlowCase and SourceManager.getFileLocSlowCase #164269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 3, 2025
Merged

[clang] Optimize SourceManager.getSpellingLocSlowCase and SourceManager.getFileLocSlowCase #164269

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f27f647
Optimize getSpellingLocSlowCase and getFileLocSlowCase
SergejSalnikov Oct 20, 2025
8a1dc7a
Merge branch 'main' into optimize
SergejSalnikov Oct 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions clang/lib/Basic/SourceManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,19 +907,23 @@ getExpansionLocSlowCase(SourceLocation Loc) const {

SourceLocation SourceManager::getSpellingLocSlowCase(SourceLocation Loc) const {
do {
FileIDAndOffset LocInfo = getDecomposedLoc(Loc);
Loc = getSLocEntry(LocInfo.first).getExpansion().getSpellingLoc();
Loc = Loc.getLocWithOffset(LocInfo.second);
const SLocEntry &Entry = getSLocEntry(getFileID(Loc));
Loc = Entry.getExpansion().getSpellingLoc().getLocWithOffset(
Loc.getOffset() - Entry.getOffset());
} while (!Loc.isFileID());
return Loc;
}

SourceLocation SourceManager::getFileLocSlowCase(SourceLocation Loc) const {
do {
if (isMacroArgExpansion(Loc))
Loc = getImmediateSpellingLoc(Loc);
else
Loc = getImmediateExpansionRange(Loc).getBegin();
const SLocEntry &Entry = getSLocEntry(getFileID(Loc));
const ExpansionInfo &ExpInfo = Entry.getExpansion();
if (ExpInfo.isMacroArgExpansion()) {
Loc = ExpInfo.getSpellingLoc().getLocWithOffset(Loc.getOffset() -
Entry.getOffset());
} else {
Loc = ExpInfo.getExpansionLocStart();
}
Comment on lines +919 to +926
Copy link
Collaborator

@AaronBallman AaronBallman Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look correct, but I guess I'm surprised there's any measurable impact on performance; are there performance measurement numbers for these changes? (We have https://llvm-compile-time-tracker.com/ to help measure that sort of thing, FWIW.)

Copy link
Contributor Author

@SergejSalnikov SergejSalnikov Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you are correct.
But it looks nicer that way. The performance depends how macro heavy is the source code. I bet you can save a few seconds on the large codebases.

Copy link
Collaborator

@AaronBallman AaronBallman Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the old code to be easier to understand what was going on, so I worry about premature optimization without some idea of measurements. CC @erichkeane @Sirraide @Endilll for some other opinions

Copy link
Contributor Author

@SergejSalnikov SergejSalnikov Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we run the performance test on this RP somehow? Although I doubt that there will be any significant change.

Copy link
Collaborator

@erichkeane erichkeane Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, both of these cases are a little tougher to read for me. I'd like to see the perf measurements. I think @Endilll knows how to start them.

Copy link
Contributor Author

@SergejSalnikov SergejSalnikov Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that PR is more relevant in the context of #163982

Copy link
Collaborator

@AaronBallman AaronBallman Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we run the performance test on this RP somehow? Although I doubt that there will be any significant change.

Unfortunately, not trivially; you can contact Nikita to have your fork added (https://llvm-compile-time-tracker.com/about.php) but that may take a while given the upcoming dev conference and folks being busy with that. I think it'd be fine if there were some local tests on whatever project you have that's super macro heavy, something as simple as "average of 1234ms before this patch and 567ms after the patch on my machine" would perhaps help.

Basically, if there's a measurable performance improvement, we can hold our nose on slightly reduced readability, but if there's no measurable performance improvement, there's less of a case for taking the patch..

Copy link
Contributor

@Endilll Endilll Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that helps, here you can follow what LLVM compiler time tracker does.

Copy link
Contributor Author

@SergejSalnikov SergejSalnikov Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the following 3 PRs together, but I don't know how to interpret the results.
#163982
#164269
#163982

full report

} while (!Loc.isFileID());
return Loc;
}
Expand Down
Loading