Page MenuHomePhabricator

Speed up preamble loading
ClosedPublic

Authored by yvvan on May 24 2017, 6:08 AM.

Details

Summary

On Windows loading preamble and caching global completion takes a while.
This is the way to improve it.
Please comment if I misuse something.

Diff Detail

Repository
rL LLVM

Event Timeline

yvvan created this revision.May 24 2017, 6:08 AM
yvvan updated this revision to Diff 100075.May 24 2017, 6:15 AM

fix member variable name, add comments

Please post the patch with full context git diff -U9999.

yvvan updated this revision to Diff 100605.May 29 2017, 5:43 AM

diff with -U9999

yvvan updated this revision to Diff 101533.Jun 6 2017, 3:25 AM

Remove global completion cache part because it's probably not always valid.

yvvan retitled this revision from Speed up preamble loading, reduce global completion cache calls to Speed up preamble loading.Jun 6 2017, 3:25 AM

Can you use a local map in TranslateStoredDiagnostics instead of storing one in the ASTUnit, or do you need to keep it around?

yvvan added a comment.Jun 6 2017, 12:47 PM

Can you use a local map in TranslateStoredDiagnostics instead of storing one in the ASTUnit, or do you need to keep it around?

The whole purpose is to use it between different TranslateStoredDiagnostics calls because it scans the same files every time. And this cache allows it to reuse the source location.

arphaman added inline comments.Jun 7 2017, 9:23 AM
include/clang/Frontend/ASTUnit.h
192 ↗(On Diff #101533)

You can use an llvm::StringMap instead.

lib/Frontend/ASTUnit.cpp
1152 ↗(On Diff #101533)

Why is clear in an else here? We always create a new SourceManager in this function, so the previously cached locations will be invalid, so shouldn't we always clear the cache before TranslateStoredDiagnostics?

yvvan added inline comments.Jun 7 2017, 10:54 AM
include/clang/Frontend/ASTUnit.h
192 ↗(On Diff #101533)

I will change that

lib/Frontend/ASTUnit.cpp
1152 ↗(On Diff #101533)

When we load diagnostics that means that preamble has not changed. Doesn't that mean that source locations can be reused? What can cause them to become invalid?
When preamble is invalidated - cache is cleared.

I can keep cache only during TranslateStoredDiagnostics calls but in that case performance improvement is way less.

But if you say that current solution is invalid I will do that

arphaman added inline comments.Jun 7 2017, 1:32 PM
lib/Frontend/ASTUnit.cpp
1152 ↗(On Diff #101533)

You're right actually, we can reuse them. We already do make that assumption that the preamble's source locations can be reused in checkAndSanitizeDiags. This code is fine then, sorry about the confusion.

You should mention in the comment for the SrcLocCache that we cache only the source locations from the preamble as we can guarantee that they will stay valid when the source manager is re-created.

yvvan updated this revision to Diff 101865.Jun 8 2017, 12:43 AM

Improve commentary. Use StringMap

arphaman accepted this revision.Jun 8 2017, 10:04 AM

LGTM with one inline request below.

I also have a question: what kind of performance benefits do you get for the preamble load times?

lib/Frontend/ASTUnit.cpp
2620 ↗(On Diff #101865)

Please move the FileID FID declaration from above into here because we only use it in this if.

This revision is now accepted and ready to land.Jun 8 2017, 10:04 AM
yvvan updated this revision to Diff 101995.Jun 8 2017, 11:41 PM

"what kind of performance benefits do you get for the preamble load times?"
In cases where many windows headers are included preamble loading takes almost the half of reparse time. With this fix time to load preamble goes towards zero.

This revision was automatically updated to reflect the committed changes.