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.

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

[do not merge] Remove PackedFingerprint #81230

Closed

Conversation

tgnottingham
Copy link
Contributor

@tgnottingham tgnottingham commented Jan 20, 2021

PackedFingerprint was added by #78646 to reduce the size and alignment of DepNodes. This lowered memory consumption at the cost of increased cycles. Since then, #79589 and #80957 have decreased the number of DepNodes that exist during a compilation session, so PackedFingerprint may not be as beneficial anymore. Let's do a perf run and find out.

r? @ghost

@tgnottingham
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 21, 2021

⌛ Trying commit 2b32c8b2e6f1939aa45d6654a3f34f2885b954cf with merge 0a3bc90310fef9bd3563b773ac96ad1720c363de...

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_middle/src/dep_graph/dep_node.rs at line 407:
     /// has been removed.
     fn extract_def_id(&self, tcx: TyCtxt<'tcx>) -> Option<DefId> {
         if self.kind.can_reconstruct_query_key() {
-            tcx.queries
-                .on_disk_cache
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_middle/src/dep_graph/dep_node.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
-                .as_ref()?
-                .def_path_hash_to_def_id(tcx, DefPathHash(self.hash))
+            tcx.queries.on_disk_cache.as_ref()?.def_path_hash_to_def_id(tcx, DefPathHash(self.hash))
             None
         }
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/tools/tidy
Build completed unsuccessfully in 0:00:18

@tgnottingham
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 21, 2021

⌛ Trying commit d4e42af with merge cf92be18e6cc5980739fdd6c7bfc48659900f777...

@bors
Copy link
Contributor

bors commented Jan 21, 2021

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

@rust-timer
Copy link
Collaborator

Queued cf92be18e6cc5980739fdd6c7bfc48659900f777 with parent a4cbb44, future comparison URL.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 21, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (cf92be18e6cc5980739fdd6c7bfc48659900f777): 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 rollup- to bors.

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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added 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 Jan 21, 2021
@tgnottingham
Copy link
Contributor Author

The instruction counts improve very slightly after reverting (deeply-nested-async is worse, but is probably bogus based on how that benchmark has behaved in the past). The cycles difference is in the noise/variance range. And the max-rss difference isn't large enough to show the through the noise.

But profiling locally under Massif, which produces less noisy results, shows that using packed fingerprints results in memory reductions of about 50MB or 4% for the style-servo-check incr-patched benchmark, for example.

Assuming that result is representative and because the other stats don't hugely favor one approach, I'm inclined to leave things as they are. But I don't have strong feelings one way or the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants