Skip to content

Lower fingerprint alignment to reduce memory consumption #78516

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

Closed

Conversation

tgnottingham
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Contributor

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2020
@tgnottingham
Copy link
Contributor Author

There's a bit of a speed versus space trade-off here. Anxious to see what a full perf run tells us about it.

In local rustc-perf benchmarks, the improvement is mostly noticeable in the larger crates like style-servo, where it reduces memory consumption by 70-80 MB. Doesn't sound like a ton, but it adds up when you're building in parallel. I should measure the impact on something like rustc_middle.

@jyn514
Copy link
Member

jyn514 commented Oct 29, 2020

---- [ui] ui/associated-types/associated-types-incomplete-object.rs stdout ----
 stderr:
------------------------------------------
thread 'rustc' panicked at 'assertion failed: eps.array_windows().all(|[a, b]| a.stable_cmp(self, b) != Ordering::Greater)', compiler/rustc_middle/src/ty/context.rs:2419:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

 query stack during panic:
#0 [object_safety_violations] determine object safety of trait `Foo`
#1 [typeck] type-checking `main`
end of query stack

Looks like this introduces a logic bug somewhere.

@tgnottingham tgnottingham force-pushed the reduce_fingerprint_alignment branch from e4cd6a4 to 08d0a98 Compare October 29, 2020 23:29
@tgnottingham
Copy link
Contributor Author

There were two issues:

  1. I had implemented PartialOrd but #[derive]'d Ord, and they were inconsistent with one another. Implemented Ord so that they're consistent. Improves performance too.

  2. The order of some diagnostics in UI test output depends on the comparison ordering of Fingerprints (DefPathHashs, really). They don't depend on it in a predictable way, since the hashes are random in a sense. But the ordering I've implemented for the sake of performance changes how some Fingerprints compare to one another, and so changes the order of a couple diagnostics. Updated the .stderr files to account for this.

@jyn514 jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 29, 2020
@camelid camelid added the I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. label Oct 30, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Oct 30, 2020

⌛ Trying commit 08d0a98 with merge 926bc74fa67402d9e95478269a50716f9f4550d1...

@camelid camelid removed the I-compiletime Issue: Problems and improvements with respect to compile times. label Oct 30, 2020
@bors
Copy link
Collaborator

bors commented Oct 30, 2020

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

@rust-timer
Copy link
Collaborator

Queued 926bc74fa67402d9e95478269a50716f9f4550d1 with parent 6bdae9e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (926bc74fa67402d9e95478269a50716f9f4550d1): 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 modify labels: +S-waiting-on-review -S-waiting-on-perf

@tgnottingham
Copy link
Contributor Author

Those are very different from the results I was getting locally. Will have to investigate.

@jyn514
Copy link
Member

jyn514 commented Oct 30, 2020

Max-rss seems to have gone down a lot though :)

@tgnottingham
Copy link
Contributor Author

One cause of the regression, and possibly all of it, is that using a [u8; 16] representation causes aliasing pessimizations to kick in. I think some of those would go away if -Z mutable-no-alias were enabled, but we're not there yet (#54878).

It's possible to work around some of the pessimizations, but it's awkward, fragile, and incomplete, so I'm abandoning this approach. Hoping to post a new PR with a better approach shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants