Better sso structures #77171

VFLashM · 2020-09-24T23:56:49Z

This change greatly expands interface of MiniSet/MiniMap and renames them because they are no longer "Mini".

rust-highfive · 2020-09-24T23:56:51Z

r? @oli-obk

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

oli-obk · 2020-09-25T10:32:23Z

@bors try @rust-timer queue

rust-timer · 2020-09-25T10:32:24Z

Awaiting bors try build completion

bors · 2020-09-25T10:32:39Z

⌛ Trying commit 1046efb1dd1e7739fea89f4717b64ede06478647 with merge 756b88018c71526ac1998d6368cd4886d88cdb73...

oli-obk · 2020-09-25T10:32:39Z

I'll review in a bit, but let's launch perfbot in parallel

bors · 2020-09-25T11:18:03Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 756b88018c71526ac1998d6368cd4886d88cdb73 (756b88018c71526ac1998d6368cd4886d88cdb73)

rust-timer · 2020-09-25T11:18:05Z

Queued 756b88018c71526ac1998d6368cd4886d88cdb73 with parent fadf025, future comparison URL.

Mark-Simulacrum · 2020-09-25T12:34:46Z

FYI, this try build might be a bit noisier or affected by more than just the PR itself, as we just applied some de-noising on the server. So it might make sense to re-create a try build, especially if perf is a blocking concern for whatever reason.

oli-obk · 2020-09-25T12:43:50Z

compiler/rustc_data_structures/src/sso/set.rs

+///   PartialEq/Eq (requires sorting the array)
+///   BitOr/BitAnd/BitXor/Sub
+#[derive(Clone)]
+pub enum SsoHashSet<T> {


Why is this not just a wrapper struct around SsoHashMap<T, ()>?

That's actually a good idea.
Sure, it's already a thin wrap, but it could be even thinner.
Let me try that.

Ah 😆 I thought there was a reason and just wanted it documented. Maybe let's do that in a separate step, since we already have a perf situation

rust-timer · 2020-09-25T13:51:32Z

Finished benchmarking try commit (756b88018c71526ac1998d6368cd4886d88cdb73): 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

oli-obk · 2020-09-25T14:56:09Z

Just to be sure that's not due to the recent perf changes:

@bors try @rust-timer queue

rust-timer · 2020-09-25T14:56:11Z

Awaiting bors try build completion

bors · 2020-09-25T14:56:19Z

⌛ Trying commit 1046efb1dd1e7739fea89f4717b64ede06478647 with merge f42f07c1222f0c090836b569b9de3e508c571e1b...

bors · 2020-09-25T15:45:04Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: f42f07c1222f0c090836b569b9de3e508c571e1b (f42f07c1222f0c090836b569b9de3e508c571e1b)

rust-timer · 2020-09-25T15:45:06Z

Queued f42f07c1222f0c090836b569b9de3e508c571e1b with parent b984ef6, future comparison URL.

rust-timer · 2020-09-25T19:27:13Z

Finished benchmarking try commit (f42f07c1222f0c090836b569b9de3e508c571e1b): 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

oli-obk · 2020-09-25T19:32:24Z

So it's not noise. Typeck deserialization from incremental caches is affected

VFLashM · 2020-09-26T02:09:21Z

I'm really surprised by the result.
I barely changes existing functions, mostly just added new ones.
I'll have to check it once again.

oli-obk · 2020-09-26T07:25:29Z

Maybe inlining changed? That can be very subtle and happen due to minor changes

VFLashM · 2020-09-26T15:49:37Z

Hmm. I can't reproduce this result locally:

Any suggestions?

Here are the changes compared to previous version:

Map::insert now returns old value if any whereas before it returned nothing
Map::get used to accept key type directly, now it accepts some type which can be borrowed as key.
Array iteration unpacks key/value tuple instead of iterating over pairs (not sure if it matters)

oli-obk · 2020-09-26T16:01:02Z

I can't tell from the code changes (where the map/set is used) how this could affect incremental cache loading times. I'm guessing it's all spurious. So, let's go with: rebase, change the set impl to just forward to the map impl, run perf again. Does that sound reasonable to you?

VFLashM · 2020-09-26T16:07:06Z

Yup, let me do just that.

It is a more descriptive name and with upcoming changes there will be nothing "mini" about them.

Now both provide almost complete API of their non-SSO counterparts.

bors · 2020-09-27T08:03:08Z

⌛ Trying commit 41942fa with merge f19b20c14696f331a43042bde62219a773d43ba4...

bors · 2020-09-27T08:47:27Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: f19b20c14696f331a43042bde62219a773d43ba4 (f19b20c14696f331a43042bde62219a773d43ba4)

rust-timer · 2020-09-27T08:47:29Z

Queued f19b20c14696f331a43042bde62219a773d43ba4 with parent c9e5e6a, future comparison URL.

rust-timer · 2020-09-27T23:02:05Z

Finished benchmarking try commit (f19b20c14696f331a43042bde62219a773d43ba4): 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

VFLashM · 2020-09-28T00:20:06Z

Sigh. Trying to reproduce it locally one more time.
Which configuration does rustc-perf use?

VFLashM · 2020-09-28T01:21:00Z

I reproduced it locally.
It only happens on stage2 compiler and I performed all my tests on stage 1.

Am I missing something?
Shouldn't stage 1 be enough to benchmark?
Given that this change is a pure performance optimization, I'd expect that stage 1 and stage 2 compilers would be the same.

oli-obk · 2020-09-28T06:27:07Z

The only thing I can come up with is that this is not a regression in this PR, but this PR is just a symptom of a regression from the beta (bootstrap) compiler to master. So master doesn't optimize your changes as well as they were optimized on beta. Can you try this out by rebasing onto the master branch from just when the mini maps were introduced and rerunning perf on stage 2? Or is that not long enough ago to make a difference?

cc @rust-lang/wg-compiler-performance @rust-lang/wg-codegen any ideas what's going on? do we have any perf regressions maybe due to an llvm upgrade or something?

VFLashM · 2020-10-02T01:13:13Z

Yeah, they were introduced just recently.

I think I narrowed it down.

When I remove generality over Q from SsoHashMap::get (as in make it accept &K for key), performance jumps back to expected levels. I thought that maybe additional borrow messes up inlining and tried marking SsoHashMap::get as #[inline]. Doing so cuts performance penalty in half, but doesn't remove it. This doesn't make sense as borrow should be an inlined NOP.

I'm out good of ideas by now. What would be the best way to inspect assembly output sans for disassembling rust?

oli-obk · 2020-10-02T09:03:56Z

#[inline] is just a hint, so I'm guessing that sometimes the forwarding to FxHashMap::get will get inlined and sometimes not. Sometimes SsoHashMap::get gets inlined, sometimes not. So there are many combinations where this can go "wrong". Maybe we merge the manually monomorphized version for now and then track making it generic again? At this point I'm wondering whether it may make sense to pull this out into a crate with its own unit tests so we can run benchmarks, valgrind and similar tooling on it.

Due to performance regression, see SsoHashMap comment.

VFLashM · 2020-10-03T01:15:16Z

Pushed a change. Removed Q generic, all methods now accept &K directly.

oli-obk · 2020-10-05T09:34:04Z

@bors try @rust-timer queue

rust-timer · 2020-10-05T09:34:06Z

Awaiting bors try build completion

bors · 2020-10-05T09:34:17Z

⌛ Trying commit d1d2184 with merge 3580ee5afaa5d6dc265216653a63bdd35ebe828c...

bors · 2020-10-05T10:20:36Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 3580ee5afaa5d6dc265216653a63bdd35ebe828c (3580ee5afaa5d6dc265216653a63bdd35ebe828c)

rust-timer · 2020-10-05T10:20:37Z

Queued 3580ee5afaa5d6dc265216653a63bdd35ebe828c with parent efbaa41, future comparison URL.

rust-timer · 2020-10-05T12:00:25Z

Finished benchmarking try commit (3580ee5afaa5d6dc265216653a63bdd35ebe828c): 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

oli-obk · 2020-10-05T12:29:39Z

@bors r+

Thanks, this looks good now. Wrt the constant that decides the array size, you could make it a const generic parameter in a follow-up PR if you think making it configurable is useful per-site.

bors · 2020-10-05T12:29:41Z

📌 Commit d1d2184 has been approved by oli-obk

bors · 2020-10-05T17:18:07Z

⌛ Testing commit d1d2184 with merge ea7e131...

bors · 2020-10-05T19:34:36Z

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing ea7e131 to master...

rust-highfive assigned oli-obk Sep 24, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2020

jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 25, 2020

oli-obk reviewed Sep 25, 2020

View reviewed changes

VFLashM added 2 commits September 26, 2020 14:30

MiniSet/MiniMap moved and renamed into SsoHashSet/SsoHashMap

5c224a4

It is a more descriptive name and with upcoming changes there will be nothing "mini" about them.

SsoHashSet/SsoHashMap API greatly expanded

0600b17

Now both provide almost complete API of their non-SSO counterparts.

VFLashM force-pushed the better_sso_structures branch from ba13505 to 848cd2a Compare September 26, 2020 19:34

SsoHashMap minor refactoring, SSO_ARRAY_SIZE introduced

92a0668

SsoHashSet/Map - genericiy over Q removed

d1d2184

Due to performance regression, see SsoHashMap comment.

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 Oct 5, 2020

bors mentioned this pull request Oct 5, 2020

Working branch-level code coverage #77080

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 5, 2020

bors merged commit ea7e131 into rust-lang:master Oct 5, 2020

rustbot added this to the 1.49.0 milestone Oct 5, 2020

Better sso structures #77171

Better sso structures #77171

Conversation

VFLashM commented Sep 24, 2020

rust-highfive commented Sep 24, 2020

oli-obk commented Sep 25, 2020

rust-timer commented Sep 25, 2020

bors commented Sep 25, 2020

oli-obk commented Sep 25, 2020

bors commented Sep 25, 2020

rust-timer commented Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

oli-obk Sep 25, 2020

Choose a reason for hiding this comment

VFLashM Sep 26, 2020

Choose a reason for hiding this comment

oli-obk Sep 26, 2020

Choose a reason for hiding this comment

rust-timer commented Sep 25, 2020

oli-obk commented Sep 25, 2020

rust-timer commented Sep 25, 2020

bors commented Sep 25, 2020

bors commented Sep 25, 2020

rust-timer commented Sep 25, 2020

rust-timer commented Sep 25, 2020

oli-obk commented Sep 25, 2020

VFLashM commented Sep 26, 2020

oli-obk commented Sep 26, 2020

VFLashM commented Sep 26, 2020

oli-obk commented Sep 26, 2020

VFLashM commented Sep 26, 2020

bors commented Sep 27, 2020

bors commented Sep 27, 2020

rust-timer commented Sep 27, 2020

rust-timer commented Sep 27, 2020

VFLashM commented Sep 28, 2020

VFLashM commented Sep 28, 2020

oli-obk commented Sep 28, 2020

VFLashM commented Oct 2, 2020

oli-obk commented Oct 2, 2020

VFLashM commented Oct 3, 2020

oli-obk commented Oct 5, 2020

rust-timer commented Oct 5, 2020

bors commented Oct 5, 2020

bors commented Oct 5, 2020

rust-timer commented Oct 5, 2020

rust-timer commented Oct 5, 2020

oli-obk commented Oct 5, 2020

bors commented Oct 5, 2020

bors commented Oct 5, 2020

bors commented Oct 5, 2020