clang: fix precompiled preamble cache misses
ClosedPublic

Authored by brauch on Tue, Jul 25, 10:51 PM.

Details

Summary

We were a) passing in a wrong file size, and b) a different set of unsaved
files on building the translation unit as compared to invoking completion.
This resulted in the precompiled preamble being rebuilt every single time.

On my test file, time spent in clang_codeCompleteAt goes down from ~700ms
to ~12ms with this change after the project is fully parsed.

It still doesn't work in header files at all.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
brauch created this revision.Tue, Jul 25, 10:51 PM
brauch updated this revision to Diff 17204.Wed, Jul 26, 8:20 AM
kfunk set the repository for this revision to R32 KDevelop.Wed, Jul 26, 8:18 PM
kfunk added a project: KDevelop.
kfunk accepted this revision.Wed, Jul 26, 8:30 PM

LGTM.

Do you think we should attempt to backport to 5.1 branch? We'd need to #ifdef the CXTranslationUnit_CreatePreambleOnFirstParse though since we still support older libclang there

languages/clang/duchain/parsesession.cpp
186

Looks good to me. CXXChainPCH seems deprecated anyway:

CXTranslationUnit_CXXChainedPCH 	
DEPRECATED: Enabled chained precompiled preambles in C++.

Note: this is a temporary option that is available only while we are testing C++ precompiled preamble support. It is deprecated.
198

Good find.

That needs libclang 3.9 though, if I understand correctly. I'm fine with bumping the dependency in master though.

This revision is now accepted and ready to land.Wed, Jul 26, 8:30 PM
mwolff requested changes to this revision.Wed, Jul 26, 9:20 PM

fix it then push it - thanks!

languages/clang/codecompletion/context.cpp
784

ugh - I hoped to not use the foreground lock in kdev code in order to remove it eventually. But fair enough for now, leave it as is.

languages/clang/duchain/parsesession.cpp
186

any idea why we added this in the first place? can you also add something to your commit message saying why you remove this now? it seems somewhat unrelated to the rest (?)

198

yeah, better add the ifdef?

languages/clang/util/clangutils.cpp
70

remove spaces inside parens

73

split onto two lines, i.e. after the first comma

languages/clang/util/clangutils.h
73

please add apidox and note that this must be done from the foreground / have the foreground locked

This revision now requires changes to proceed.Wed, Jul 26, 9:20 PM
brauch marked 2 inline comments as done.Wed, Jul 26, 10:03 PM

Thanks for the reviews, I'll submit a fixed version tomorrow. I would not backport this tbh, but instead release 5.2 as soon as possible; I feel like the implications are relatively far-reaching and that shouldn't be in a point release. People who want to try the fix can use our 5.2 AppImage (with up-to-date clang).

languages/clang/duchain/parsesession.cpp
186

It was supposed to tackle a similar problem but is now deprecated, according to documentation. I assume it does nothing right now anyways, so I removed it while touching that code.

198

Yes, I'll ifdef that

kfunk added a comment.Thu, Jul 27, 6:23 AM

Thanks for the reviews, I'll submit a fixed version tomorrow. I would not backport this tbh, but instead release 5.2 as soon as possible; I feel like the implications are relatively far-reaching and that shouldn't be in a point release. People who want to try the fix can use our 5.2 AppImage (with up-to-date clang).

Agreed.

This revision was automatically updated to reflect the committed changes.
brauch marked an inline comment as done.