Uh oh!
There was an error while loading. Please reload this page.
Increase Span from 4 bytes to 8 bytes. - #59693
Conversation
I was pleasantly surprised how large the improvements are. Some instruction counts results for Check builds: Here is part of a Cachegrind diff showing the changes for a Check-CleanIncr build of Mostly TLS stuff and interning stuff, which makes sense. |
nnethercote
commented
Apr 4, 2019
@bors try |
bors
commented
Apr 4, 2019
Increase `Span` from 4 bytes to 8 bytes. This increases the size of some important types, such as `ast::Expr` and `mir::Statement`. However, it drastically reduces how much the interner is used, and the fields are more natural sizes that don't require bit operations to extract. As a result, instruction counts drop across a range of workloads, by as much as 10% for `script-servo` incremental builds. Peak memory usage goes up a little for some cases, but down by more for some other cases -- as much as 18% for non-incremental builds of `packed-simd`. The commit also: - removes the `repr(packed)`, because it has negligible effect, but can cause undefined behaviour; - replaces explicit impls of common traits (`Copy`, `PartialEq`, etc.) with derived ones. r? @petrochenkov
nnethercote
commented
Apr 4, 2019
It's interesting that TLS is so expensive. Makes me wonder if other TLS data (e.g. symbols) could be done some other way. |
nnethercote
commented
Apr 4, 2019
Actually, I don't understand |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
petrochenkov
commented
Apr 4, 2019
Previous discussion for reference - #58458. |
petrochenkov
commented
Apr 4, 2019
That's entirely @Zoxc's creation, I don't understand our threading setup. |
petrochenkov
commented
Apr 4, 2019
@nnethercote |
Zoxc
commented
Apr 4, 2019
Is the interned case uncommon enough to throw |
bors
commented
Apr 4, 2019
☀️ Try build successful - checks-travis |
nnethercote
commented
Apr 4, 2019
Yes! It's very rare for len to hit 16 bits in any code. But ctxt depends on the crate size, so in very large crates a decent number of ctxts could require 16 bits. In fact, ideally I'd probably use something like 11 bits for len and 20 for ctxt, but those sizes aren't as natural. I'll add a comment about this. |
nnethercote
commented
Apr 4, 2019
It's fine by me, though I thought they were generally frowned upon in Rust code... |
nnethercote
commented
Apr 4, 2019
@rust-timer build cfa7765 |
rust-timer
commented
Apr 4, 2019
Success: Queued cfa7765 with parent f717b58, comparison URL. |
That's what I expected, ctxt behaves in a similar way to base and can just grow and grow, and that's different from len. |
rust-timer
commented
Apr 5, 2019
Finished benchmarking try commit cfa7765 |
nnethercote
commented
Apr 5, 2019
The CI results are even better than what I saw locally, yay. |
This increases the size of some important types, such as `ast::Expr` and `mir::Statement`. However, it drastically reduces how much the interner is used, and the fields are more natural sizes that don't require bit operations to extract. As a result, instruction counts drop across a range of workloads, by as much as 12% for incremental "check" builds of `script-servo`. Peak memory usage goes up a little for some cases, but down by more for some other cases -- as much as 18% for non-incremental builds of `packed-simd`. The commit also: - removes the `repr(packed)`, because it has negligible effect, but can cause undefined behaviour; - replaces explicit impls of common traits (`Copy`, `PartialEq`, etc.) with derived ones.
nnethercote
commented
Apr 5, 2019
@petrochenkov: I have updated the comments as per the suggestions above. And thanks for suggesting 64-bits in the first place! :) |
The previous measurements (#44646 (comment)) for 64 bits looked very different (that's why 32 was selected in the first place). I wonder what changed since Sep 2017. (I'll review the code this evening.) |
petrochenkov
commented
Apr 6, 2019
Code LGTM, but I'd want to run #59749 through perf before merging this. |
petrochenkov
commented
Apr 12, 2019
I see, then I'm not sure why it's per-session and not global. |
Zoxc
commented
Apr 12, 2019
There can be multiple rustc sessions in a process. RLS and rustdoc does that. |
I mean, why can't multiple sessions share a single interner? |
Mark-Simulacrum
commented
Apr 12, 2019
@bors rollup |
…enkov Increase `Span` from 4 bytes to 8 bytes. This increases the size of some important types, such as `ast::Expr` and `mir::Statement`. However, it drastically reduces how much the interner is used, and the fields are more natural sizes that don't require bit operations to extract. As a result, instruction counts drop across a range of workloads, by as much as 10% for `script-servo` incremental builds. Peak memory usage goes up a little for some cases, but down by more for some other cases -- as much as 18% for non-incremental builds of `packed-simd`. The commit also: - removes the `repr(packed)`, because it has negligible effect, but can cause undefined behaviour; - replaces explicit impls of common traits (`Copy`, `PartialEq`, etc.) with derived ones. r? @petrochenkov
nnethercote
commented
Apr 12, 2019
It's too late now, but for next time: PRs with significant perf effects are better landed on their own, rather than in a rollup, to keep the perf data clear. For example, if some other PR in the rollup introduces a perf regression, that would be masked by the improvement in this PR. |
petrochenkov
commented
Apr 12, 2019
@bors rollup- |
Centril
commented
Apr 13, 2019
@bors p=1 |
Increase `Span` from 4 bytes to 8 bytes. This increases the size of some important types, such as `ast::Expr` and `mir::Statement`. However, it drastically reduces how much the interner is used, and the fields are more natural sizes that don't require bit operations to extract. As a result, instruction counts drop across a range of workloads, by as much as 10% for `script-servo` incremental builds. Peak memory usage goes up a little for some cases, but down by more for some other cases -- as much as 18% for non-incremental builds of `packed-simd`. The commit also: - removes the `repr(packed)`, because it has negligible effect, but can cause undefined behaviour; - replaces explicit impls of common traits (`Copy`, `PartialEq`, etc.) with derived ones. r? @petrochenkov
bors
commented
Apr 14, 2019
bors
commented
Apr 14, 2019
💔 Test failed - checks-travis |
rust-highfive
commented
Apr 14, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nnethercote
commented
Apr 14, 2019
"warning: spurious network error" @bors retry |
bors
commented
Apr 14, 2019
Increase `Span` from 4 bytes to 8 bytes. This increases the size of some important types, such as `ast::Expr` and `mir::Statement`. However, it drastically reduces how much the interner is used, and the fields are more natural sizes that don't require bit operations to extract. As a result, instruction counts drop across a range of workloads, by as much as 10% for `script-servo` incremental builds. Peak memory usage goes up a little for some cases, but down by more for some other cases -- as much as 18% for non-incremental builds of `packed-simd`. The commit also: - removes the `repr(packed)`, because it has negligible effect, but can cause undefined behaviour; - replaces explicit impls of common traits (`Copy`, `PartialEq`, etc.) with derived ones. r? @petrochenkov
bors
commented
Apr 14, 2019
☀️ Test successful - checks-travis, status-appveyor |
eddyb
commented
Apr 16, 2019
Btw, there may be a way to fit The reason is that we could allocate new "files" (or maybe even just one chunk, since it should often suffice) for the version with a And so two values (file + |
swc_common: - apply patch from rust-lang/rust#59693 swc: - use &Options instead of Options - configures commons::CM - exposes `handler`
This increases the size of some important types, such as
ast::Exprandmir::Statement. However, it drastically reduces how much the interneris used, and the fields are more natural sizes that don't require bit
operations to extract.
As a result, instruction counts drop across a range of workloads, by as
much as 10% for
script-servoincremental builds.Peak memory usage goes up a little for some cases, but down by more for
some other cases -- as much as 18% for non-incremental builds of
packed-simd.The commit also:
repr(packed), because it has negligible effect, but cancause undefined behaviour;
Copy,PartialEq, etc.)with derived ones.
r? @petrochenkov