Skip to content

/rust

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.

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

incr.comp.: Load cached diagnostics lazily and allow more things in the cache. #46338

Merged
merged 7 commits into from Dec 1, 2017

Conversation

Reviewers

@nikomatsakis nikomatsakis

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
5 participants
@michaelwoerister @bors @nikomatsakis @Mark-Simulacrum @kennytm
@michaelwoerister
Copy link
Contributor

michaelwoerister commented Nov 28, 2017 •
edited

This PR implements makes two changes:

  1. Diagnostics are loaded lazily from the incr. comp. cache now. This turned out to be necessary for correctness because diagnostics contain Span values and deserializing those requires that the source file they point to is still around in the current compilation session. Obviously this isn't always the case. Loading them lazily allows for never touching diagnostics that are not valid anymore.
  2. The compiler can now deal with there being no cache entry for a given query invocation. Before, all query results of a cacheable query were always expected to be present in the cache. Now, the compiler can fall back to re-computing the result if there is no cache entry found. This allows for caching things that we cannot force from dep-node (like the symbol_name query). In such a case we'll just have a "best effort" caching strategy.

This PR is based on #46301 (=first 2 commits), so please don't merge until that has landed. The rest of the commits are ready for review though.

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Nov 28, 2017

@kennytm kennytm added the S-blocked label Nov 28, 2017

@nikomatsakis

nikomatsakis reviewed Nov 29, 2017
View changes

src/librustc/dep_graph/graph.rs Outdated
dep_node: &DepNode)
-> Option<DepNodeIndex> {
pub fn try_mark_green<'a, 'tcx>(&self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Nov 29, 2017

Contributor

Nit: use '_' instead of 'a here

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Nov 29, 2017

Author Contributor

That's a thing now?
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 30, 2017

☔️ The latest upstream changes (presumably #46299) made this pull request unmergeable. Please resolve the merge conflicts.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Dec 1, 2017

r=me

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:lazy-diagnostics branch from 592e969 to 410f850 Dec 1, 2017

@michaelwoerister
incr.comp.: Fix merge fallout.
Loading status checks…
966eead

@michaelwoerister michaelwoerister referenced this pull request Dec 1, 2017

Closed

incr.comp.: Fix Span hashing in various ways #46301

@michaelwoerister michaelwoerister changed the title [DO NOT MERGE YET] incr.comp.: Load cached diagnostics lazily and allow more things in the cache. incr.comp.: Load cached diagnostics lazily and allow more things in the cache. Dec 1, 2017

@michaelwoerister michaelwoerister removed the S-blocked label Dec 1, 2017

@kennytm kennytm added the S-waiting-on-review label Dec 1, 2017

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Dec 1, 2017

OK, travis passed.

@bors r=nikomatsakis p=1
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 1, 2017

📌 Commit 966eead has been approved by nikomatsakis

@michaelwoerister michaelwoerister added S-waiting-on-bors and removed S-waiting-on-review labels Dec 1, 2017

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 1, 2017

⌛️ Testing commit 966eead with merge 6805b01...

bors added a commit that referenced this pull request Dec 1, 2017

@bors
Auto merge of #46338 - michaelwoerister:lazy-diagnostics, r=nikomatsakis 
incr.comp.: Load cached diagnostics lazily and allow more things in the cache.

This PR implements makes two changes:
1. Diagnostics are loaded lazily from the incr. comp. cache now. This turned out to be necessary for correctness because diagnostics contain `Span` values and deserializing those requires that the source file they point to is still around in the current compilation session. Obviously this isn't always the case. Loading them lazily allows for never touching diagnostics that are not valid anymore.
2. The compiler can now deal with there being no cache entry for a given query invocation. Before, all query results of a cacheable query were always expected to be present in the cache. Now, the compiler can fall back to re-computing the result if there is no cache entry found. This allows for caching things that we cannot force from dep-node (like the `symbol_name` query). In such a case we'll just have a "best effort" caching strategy.

~~This PR is based on #46301 (=first 2 commits), so please don't merge until that has landed. The rest of the commits are ready for review though.~~

r? @nikomatsakis
Loading status checks…
6805b01

@bors bors referenced this pull request Dec 1, 2017

Merged

MIR: change "lvalue" terminology to "place". #46425

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 1, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6805b01 to master...

@bors bors merged commit 966eead into rust-lang:master Dec 1, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@bors bors referenced this pull request Dec 1, 2017

Merged

Use PathBuf instead of String where applicable #46335

@Mark-Simulacrum

This comment has been minimized.

Sign in to view
Copy link
Member

Mark-Simulacrum commented Dec 3, 2017

This appears to have caused a slight regression in performance -- http://perf.rust-lang.org/compare.html?start=e3ed21272d5f50c37c09f2a7f06c40f56b6ac298&end=6805b016efdfcd99e706003fab1336df73f6811b&stat=wall-time -- was that expected?
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Dec 4, 2017

Bummer, I would have hoped that the slightly more efficient span hashing implementation would make things faster. But it seems that also encoding and decoding expansion contexts adds some noticeable overhead for some crates. Fortunately it doesn't seem too bad -- except for crates.io again. This one seems to be rather sensitive to span related changes.

@michaelwoerister michaelwoerister referenced this pull request Dec 12, 2017

Merged

incr.comp.: Speed up span hashing by caching expansion context hashes. #46562

bors added a commit that referenced this pull request Dec 14, 2017

@bors
Auto merge of #46562 - michaelwoerister:faster-span-hashing, r=eddyb 
incr.comp.: Speed up span hashing by caching expansion context hashes.

This PR fixes the performance regressions from #46338.

r? @nikomatsakis
Loading status checks…
3fc7f85
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.