incr-comp: hash and serialize span end line/column #76256
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @Aaron1011 |
|
It's a little unsettling at first that the hash should contain both the length and end line/column, because we can derive one from the other. It seems redundant. But that's only true when we have a source map from which to derive that info. A span in isolation is incomplete without both the length and the end location. That's my intuition, anyway. But this raises the question of whether both should be serialized. Since we do have a source map when we create the in-memory spans, we don't actually need both pieces of information. Except that having both allows us to check that the information derived from the source map is consistent with what we read from disk. I'm fairly certain it's a bug if not. We probably shouldn't be reusing a span that is inconsistent with what we derive from the source map. So I've serialized both the end location and length in this PR, but I'm interested to hear what others think about the issue. |
|
@bors try @rust-timer queue Let's get some sense of how much of a performance hit it is to hash both -- if it's not, then I'm inclined to say we should :) |
|
Awaiting bors try build completion |
|
⌛ Trying commit d5b6701ccc8217f75d78da34279264f687fc3cdd with merge a58d6a897ef3f0383e1fcc53cb87c6a2c8c64145... |
tgnottingham
force-pushed
the
issue-74890
branch
from
d5b6701
to
01aff5c
Compare
|
@Mark-Simulacrum, I force pushed an update to the commit message. :) Does the perf run need to be requeued? |
|
Sort of. @rust-timer build a58d6a897ef3f0383e1fcc53cb87c6a2c8c64145 The try build completed, but bors currently loses track and doesn't print the "Try build finished" message if you force push. |
|
Queued a58d6a897ef3f0383e1fcc53cb87c6a2c8c64145 with parent 95815c9, future comparison URL. |
jyn514
added
A-incr-comp
|
Finished benchmarking try commit (a58d6a897ef3f0383e1fcc53cb87c6a2c8c64145): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
Okay, looks like there are some optimization opportunities here, which is great, because that's an area I'd like to work on. :) Looking into some small, local optimizations to start, just to see if we can get this PR performance-neutral. |
|
@tgnottingham Ping from triage, what's the status of this? |
|
@crlf0710 I have a couple of performance improvements in the works. I can probably get them out within the week. |
tgnottingham
force-pushed
the
issue-74890
branch
from
01aff5c
to
62e7a00
Compare
|
I made some changes that should help with the performance hit of the additional line/column lookup, and some minor changes to hashing. The last commit, which added a I'm making some larger changes on the hashing side, but they're significant enough that they should go into a separate PR. Hopefully that will come later this week. |
tgnottingham
force-pushed
the
issue-74890
branch
from
62e7a00
to
b8b3cd7
Compare
|
Hope to have the hash performance improvements out tomorrow or the next day. |
|
By the way, the hashing performance improvements are up for review in #77476. |
|
☔ The latest upstream changes (presumably #77687) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
tgnottingham
force-pushed
the
issue-74890
branch
from
b8b3cd7
to
b3a97db
Compare
nikomatsakis
added
the
S-blocked
|
I've marked this S-blocked, from reading the PR thread it's my impression that this PR is blocked on some performance improvements. @tgnottingham is that correct? |
|
@nikomatsakis, yes. |
tgnottingham
force-pushed
the
issue-74890
branch
from
b3a97db
to
1a1b907
Compare
bors
added
S-waiting-on-bors
|
⌛ Testing commit b71e627 with merge 846543277ca38eb23967c561ec9e1762a5cd2f41... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors r- the new test failed on https://github.com/rust-lang-ci/rust/runs/1368962233 |
bors
added
S-waiting-on-author
|
@jonas-schievink, we're getting multiple linker errors from a new run-make test I added. I saw that you were responsible for this line in another run-make file: Do you happen to know what I need to change in my run-make test to make it pass CI? I didn't think I was doing anything novel. |
Co-authored-by: Jonas Schievink <[email protected]>
tgnottingham
force-pushed
the
issue-74890
branch
from
2cc952b
to
dac57e6
Compare
|
@bors r+ |
|
📌 Commit dac57e6 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Hash both the length and the end location (line/column) of a span. If we
hash only the length, for example, then two otherwise equal spans with
different end locations will have the same hash. This can cause a
problem during incremental compilation wherein a previous result for a
query that depends on the end location of a span will be incorrectly
reused when the end location of the span it depends on has changed. A
similar analysis applies if some query depends specifically on the
length of the span, but we only hash the end location. So hash both.
Fix #46744, fix #59954, fix #63161, fix #73640, fix #73967, fix #74890, fix #75900
See #74890 for a more in-depth analysis.
I haven't thought about what other problems this root cause could be responsible for. Please let me know if anything springs to mind. I believe the issue has existed since the inception of incremental compilation.