Page MenuHomePhabricator

[clangd] Do not rebuild AST if inputs have not changed
ClosedPublic

Authored by ilya-biryukov on Jul 25 2018, 3:49 AM.

Details

Summary

If the contents are the same, the update most likely comes from the
fact that compile commands were invalidated. In that case we want to
avoid rebuilds in case the compile commands are actually the same.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Jul 25 2018, 3:49 AM
ioeric added inline comments.Jul 25 2018, 7:43 AM
clangd/TUScheduler.cpp
357 ↗(On Diff #157217)

nit: (OldPreamble == NewPreamble)

Do you intend to compare the shared pointer instead of the values?

360 ↗(On Diff #157217)

Why reset?

373 ↗(On Diff #157217)

Do we choose to ignore this because it's rare? What are non-preamble #includes?

376 ↗(On Diff #157217)

I think this can be a log.

test/clangd/extra-flags.test
26 ↗(On Diff #157217)

Why this change?

ilya-biryukov marked an inline comment as done.
  • Use log instead of vlog
  • Add parens around &&
ilya-biryukov added inline comments.Jul 25 2018, 7:56 AM
clangd/TUScheduler.cpp
357 ↗(On Diff #157217)

Yes, this is intentional. buildPreamble will either return the old one, if it can be reused, or build the new one.

360 ↗(On Diff #157217)

We don't need the old preamble at this point, so we give it a chance to die (if there are no more references).
Note that there's an expensive operation that follows (building the AST), so removing the preamble before it seems like a win

373 ↗(On Diff #157217)

Exactly, we don't have the code to track and check for all accessed files.
This should be rare, hopefully won't show up in practice.

test/clangd/extra-flags.test
26 ↗(On Diff #157217)

The test expects two publishDiagnostics, but it won't get the second one since if the contents of the file are the same :-)
So I altered the contents a bit to make sure we get the second result too

ioeric accepted this revision.Jul 25 2018, 8:46 AM

lg

clangd/TUScheduler.cpp
360 ↗(On Diff #157217)

sg. and do we guard this with mutex because the same old preamble data can be accessed by other threads? might worth a comment.

This revision is now accepted and ready to land.Jul 25 2018, 8:46 AM
simark added a subscriber: simark.Jul 25 2018, 1:01 PM

Thanks, that's simple and efficient. I'll update D49267 (to call reparseOpenFiles once again) once this is merged.

  • Move OldPreamble.reset() out of the lock, add a comment
ilya-biryukov added inline comments.Jul 26 2018, 2:20 AM
clangd/TUScheduler.cpp
360 ↗(On Diff #157217)

Guarding with mutex is not actually required. Moved it out of the locked section and added a comment, thanks!

This revision was automatically updated to reflect the committed changes.

Thanks, that's simple and efficient. I'll update D49267 (to call reparseOpenFiles once again) once this is merged.

LG, thanks!