Skip to content
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

Jump to bottom

incr. comp.: Don't compute HIR hash values twice #35549

Closed
michaelwoerister opened this issue Aug 9, 2016 · 2 comments · Fixed by #35854
Closed

incr. comp.: Don't compute HIR hash values twice #35549

michaelwoerister opened this issue Aug 9, 2016 · 2 comments · Fixed by #35854
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@michaelwoerister
Copy link
Member

When loading the dependency graph during incremental compilation, we already have to compute the ICH of most (or all?) HIR nodes in the current codebase. These hashes could be cached instead of calculating them again later when saving the dep-graph back out.

cc @nikomatsakis
@michaelwoerister michaelwoerister added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-incr-comp Area: Incremental compilation labels Aug 9, 2016
@nikomatsakis
Copy link
Contributor

One quite note: It's not clear to me that caching this is better...but it probably is. Using more memory tends to not cost that much in terms of running time, ultimately.

@nikomatsakis
Copy link
Contributor

It turns out that fixing this bug would also fix #35593 -- that is caused by the fact that typeck rewrites the result of name resolution in place, and hence the name resolution hashes after typeck are different than the ones from before. =(

@nikomatsakis nikomatsakis mentioned this issue Aug 18, 2016
Closed
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Aug 20, 2016
@nikomatsakis
compute and cache HIR hashes at beginning
6bd80d1 
This avoids the compile-time overhead of computing them twice.  It also fixes
an issue where the hash computed after typeck is differen than the hash before,
because typeck mutates the def-map in place.

Fixes rust-lang#35549.
Fixes rust-lang#35593.
@nikomatsakis nikomatsakis mentioned this issue Aug 20, 2016
Merged
bors added a commit that referenced this issue Aug 23, 2016
@bors
Auto merge of #35854 - nikomatsakis:incr-comp-cache-hash-35549, r=mw
012f45e 
compute and cache HIR hashes at beginning

This avoids the compile-time overhead of computing them twice.  It also fixes
an issue where the hash computed after typeck is differen than the hash before,
because typeck mutates the def-map in place.

Fixes #35549.
Fixes #35593.

Some performance measurements suggest this `HashesMap` is very small in memory (unobservable via `-Z time-passes`) and very cheap to construct. I do see some (very minor) performance wins in the incremental case after the first run -- the first run costs more because loading the dep-graph didn't have any hashing to do in that case. Example timings from two runs  of `libsyntex-syntax` -- the (1) indicates first run, (2) indicates second run, and (*) indicates both together:

| Phase | Master | Branch |
| ---- | ---- | ---- |
| compute_hashes_map (1) | N/A | 0.343 |
| load_dep_graph (1) | 0 | 0 |
| serialize dep graph (1) | 4.190 | 3.920 |
| total (1) | 4.190 | 4.260 |
| compute_hashes_map (2) | N/A | 0.344 |
| load_dep_graph (2) | 0.592 | 0.252 |
| serialize dep graph (2) | 4.119 | 3.779 |
| total (2) | 4.71 | 4.375 |
| total (*) | 8.9 | 8.635 |

r? @michaelwoerister
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

compute and cache HIR hashes at beginning nikomatsakis/rust
2 participants
@nikomatsakis @michaelwoerister