EXPERIMENTAL: Hash spans unconditionally during incr. comp. - #46490
Closed
michaelwoerister wants to merge 3 commits into
Closed
EXPERIMENTAL: Hash spans unconditionally during incr. comp.#46490michaelwoerister wants to merge 3 commits into
michaelwoerister wants to merge 3 commits into
Conversation
Contributor
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
Member
Author
|
@bors try |
Collaborator
bors
added a commit
that referenced
this pull request
Dec 4, 2017
EXPERIMENTAL: Hash spans unconditionally during incr. comp. It would be good for reliability if we could do without all this special-casing around change detection for spans. This PR removes most extra logic around span hashing and is meant to gauge how much of a performance regression this would cause. Much of the potential performance loss should be recoverable by better factoring queries -- but that's a medium term undertaking and should not block incr. comp. stabilization. cc @nikomatsakis @Mark-Simulacrum
Member
Author
|
cc #46303 |
kennytm
added
S-waiting-on-review
…pstream crates in the SVH.
Collaborator
|
☀️ Test successful - status-travis |
Member
Author
|
@bors try |
Collaborator
bors
added a commit
that referenced
this pull request
Dec 4, 2017
EXPERIMENTAL: Hash spans unconditionally during incr. comp. It would be good for reliability if we could do without all this special-casing around change detection for spans. This PR removes most extra logic around span hashing and is meant to gauge how much of a performance regression this would cause. Much of the potential performance loss should be recoverable by better factoring queries -- but that's a medium term undertaking and should not block incr. comp. stabilization. cc @nikomatsakis @Mark-Simulacrum
Member
Author
|
OK, so the results from http://perf.rust-lang.org/compare.html?start=fdfbcf85d55da97ed1a00823510b876018047aaf&end=fe077faa64169fdb9a5710789e6875110734f316&stat=instructions%3Au are pretty much as expected:
I interpret these big regressions as some codegen units being invalidated that weren't invalidated before for the same change set. Factoring spans out of the HIR, MIR, and Attributes should bring the lost performance back, but is not a small undertaking. I've pushed some more commits that enable caching of many high-impact queries and started another try-build. This additional set of queries can only be cached because of the accurate span hashing. Let's see how performance looks for that. |
Collaborator
|
💥 Test timed out |
Member
Author
|
@bors try |
Member
Author
|
@bors retry |
Collaborator
bors
added a commit
that referenced
this pull request
Dec 5, 2017
EXPERIMENTAL: Hash spans unconditionally during incr. comp. It would be good for reliability if we could do without all this special-casing around change detection for spans. This PR removes most extra logic around span hashing and is meant to gauge how much of a performance regression this would cause. Much of the potential performance loss should be recoverable by better factoring queries -- but that's a medium term undertaking and should not block incr. comp. stabilization. cc @nikomatsakis @Mark-Simulacrum
Collaborator
|
☀️ Test successful - status-travis |
Member
Author
|
This is superseded by #46556. Thanks for the performance measurements, @Mark-Simulacrum! |
bors
added a commit
that referenced
this pull request
Dec 8, 2017
…akis incr.comp.: Enable query result caching for many more queries Newly cached queries are: * const_is_rvalue_promotable_to_static * trans_fulfill_obligation * optimized_mir * unsafety_check_result * borrowck * mir_borrowck * mir_const_qualif * contains_extern_indicator * def_symbol_name * symbol_name This also includes the stricter `Span` hashing first mentioned in #46490, which will lead to more false positives in release builds but overall is more correct -- and necessary for caching MIR. Hopefully we will soon be able to reduce the rate of false positives again by factoring `Span` out of MIR. r? @nikomatsakis
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It would be good for reliability if we could do without all this special-casing around change detection for spans. This PR removes most extra logic around span hashing and is meant to gauge how much of a performance regression this would cause. Much of the potential performance loss should be recoverable by better factoring queries -- but that's a medium term undertaking and should not block incr. comp. stabilization.
cc @nikomatsakis @Mark-Simulacrum