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

Lazily decode SourceFile from metadata #100209

Merged
merged 8 commits into from
Aug 19, 2022
Merged

Lazily decode SourceFile from metadata #100209

merged 8 commits into from
Aug 19, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Aug 6, 2022 •
edited
Loading

Currently, source files from foreign crates are decoded up-front from metadata.
Spans from those crates were matched with the corresponding source using binary search among those files.

This PR changes the strategy by matching spans to files during encoding. This allows to decode source files on-demand, instead of up-front. The on-disk format for spans becomes: <tag> <position from start of file> <length> <file index> <crate (if foreign file)>.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 6, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Aug 6, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 6, 2022
@bors
Copy link
Contributor

bors commented Aug 6, 2022

⌛ Trying commit 16ba778 with merge 7727698a1761dc5ff29971bb098c1fd11cd70852...

@bors
Copy link
Contributor

bors commented Aug 6, 2022

☀️ Try build successful - checks-actions
Build commit: 7727698a1761dc5ff29971bb098c1fd11cd70852 (7727698a1761dc5ff29971bb098c1fd11cd70852)

@rust-timer
Copy link
Collaborator

Queued 7727698a1761dc5ff29971bb098c1fd11cd70852 with parent 44bd81d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7727698a1761dc5ff29971bb098c1fd11cd70852): comparison url.

Instruction count

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 0.3% 0.4% 9
Regressions 😿
(secondary)		 1.0% 1.3% 4
Improvements 🎉
(primary)		 -2.2% -18.2% 43
Improvements 🎉
(secondary)		 -3.2% -17.1% 135
All 😿🎉 (primary) -1.7% -18.2% 52

Max RSS (memory usage)

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 1.0% 1.0% 1
Regressions 😿
(secondary)		 2.3% 4.3% 4
Improvements 🎉
(primary)		 -1.4% -2.8% 3
Improvements 🎉
(secondary)		 -3.6% -7.8% 16
All 😿🎉 (primary) -0.8% -2.8% 4

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 3.7% 3.7% 1
Improvements 🎉
(primary)		 -9.6% -10.7% 4
Improvements 🎉
(secondary)		 -4.8% -9.9% 41
All 😿🎉 (primary) -9.6% -10.7% 4

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 7, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Aug 7, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 7, 2022
@bors
Copy link
Contributor

bors commented Aug 7, 2022

⌛ Trying commit bacb4db with merge 72ee1407cb57fa4b7a2ca9feaae9cb0d325a0279...

@bors
Copy link
Contributor

bors commented Aug 7, 2022

☀️ Try build successful - checks-actions
Build commit: 72ee1407cb57fa4b7a2ca9feaae9cb0d325a0279 (72ee1407cb57fa4b7a2ca9feaae9cb0d325a0279)

@rust-timer
Copy link
Collaborator

Queued 72ee1407cb57fa4b7a2ca9feaae9cb0d325a0279 with parent 5651759, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (72ee1407cb57fa4b7a2ca9feaae9cb0d325a0279): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 0.2% 0.2% 2
Regressions 😿
(secondary)		 1.1% 1.4% 8
Improvements 🎉
(primary)		 -1.8% -18.2% 59
Improvements 🎉
(secondary)		 -2.9% -17.1% 152
All 😿🎉 (primary) -1.7% -18.2% 61

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 1.0% 1.0% 1
Regressions 😿
(secondary)		 2.3% 2.6% 6
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -3.2% -6.1% 10
All 😿🎉 (primary) 1.0% 1.0% 1

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -8.5% -11.8% 5
Improvements 🎉
(secondary)		 -4.8% -11.1% 42
All 😿🎉 (primary) -8.5% -11.8% 5

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Aug 7, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Aug 7, 2022

r? compiler

Mark-Simulacrum
Mark-Simulacrum reviewed Aug 7, 2022
View reviewed changes
compiler/rustc_metadata/src/rmeta/encoder.rs
required_source_files: Option<GrowableBitSet<usize>>,
// `SourceFile` into the `FxIndexSet`.
// The order inside the `FxIndexSet` is used as on-disk
// order of `SourceFiles`, and encoded inside `Span`s.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth making this order sensitive like that? I could imagine some kind of poor effects on deterministic metadata contents -- maybe we can emit in order or even sorted by (remapped) file path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of the SourceFiles here should be exactly the order in which spans are encoded. Therefore, a change in this order can only be caused by a change in encoded spans. As we can't really know which SourceFiles we will need in the end, I don't see how we can pre-sort them.

compiler/rustc_span/src/source_map.rs Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 19, 2022

📌 Commit 0d41f91 has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2022
@bors
Copy link
Contributor

bors commented Aug 19, 2022

⌛ Testing commit 0d41f91 with merge 468887e...

@bors
Copy link
Contributor

bors commented Aug 19, 2022

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 468887e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2022
@bors bors merged commit 468887e into rust-lang:master Aug 19, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 19, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (468887e): comparison url.

Instruction count

  • Primary benchmarks: ✅ relevant improvements found
  • Secondary benchmarks: ✅ relevant improvements found
mean1 max count2
Regressions ❌
(primary)		 0.2% 0.2% 2
Regressions ❌
(secondary)		 0.7% 0.9% 3
Improvements ✅
(primary)		 -1.6% -19.6% 75
Improvements ✅
(secondary)		 -3.0% -18.3% 158
All ❌✅ (primary) -1.6% -19.6% 77

Max RSS (memory usage)

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions ❌
(primary)		 1.7% 2.2% 5
Regressions ❌
(secondary)		 3.3% 4.2% 7
Improvements ✅
(primary)		 -2.1% -2.1% 1
Improvements ✅
(secondary)		 -2.6% -3.4% 12
All ❌✅ (primary) 1.1% 2.2% 6

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: ✅ relevant improvements found
mean1 max count2
Regressions ❌
(primary)		 4.0% 4.0% 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -8.4% -9.8% 4
Improvements ✅
(secondary)		 -4.9% -10.0% 38
All ❌✅ (primary) -6.0% -9.8% 5

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@cjgillot cjgillot deleted the source-file-index branch August 20, 2022 07:55
@klensy klensy mentioned this pull request Aug 20, 2022
Merged
@frewsxcv
Copy link
Member

Potentially relevant reddit thread: https://www.reddit.com/r/rust/comments/wt5vr9/compile_time_wins_today/

@klensy
Copy link
Contributor

klensy commented Aug 21, 2022

This changes encoding for Span in rmeta, shouldn't encoding in rustc_query_impl be changed too?

@cjgillot
Copy link
Contributor Author

@klensy no. metadata and incremental cache have different formats, on purpose.
We can rely on the order given in the metadata file for the program behaviour. Everything will be recompiled when this order change.
On the other hand, the order in the incremental cache can change unpredictably between two compiler runs. (I mean: the compiler does not predict the programmers' modifications to the code.) So this format needs to recognize stuff whose order may have changed.
Does it answer your question?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@estebank estebank

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.
8 participants
@cjgillot @rust-timer @bors @estebank @frewsxcv @klensy @Mark-Simulacrum @rustbot