Make intern_lazy_const actually intern its argument.#58207
Make intern_lazy_const actually intern its argument.#58207bors merged 1 commit intorust-lang:masterfrom
intern_lazy_const actually intern its argument.#58207Conversation
Currently it just unconditionally allocates it in the arena. For a "Clean Check" build of the the `packed-simd` benchmark, this change reduces both the `max-rss` and `faults` counts by 59%; it slightly (~3%) increases the instruction counts but the `wall-time` is unchanged. For the same builds of a few other benchmarks, `max-rss` and `faults` drop by 1--5%, but instruction counts and `wall-time` changes are in the noise. Fixes rust-lang#57432, fixes rust-lang#57829.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
⌛ Trying commit f2871a9 with merge 75a5b718ee128eee283eb50bc379c66401add1e5... |
|
I find it amusing that this removes |
|
☀️ Test successful - checks-travis |
|
@rust-timer build 75a5b718ee128eee283eb50bc379c66401add1e5 |
|
Success: Queued 75a5b718ee128eee283eb50bc379c66401add1e5 with parent 65e647c, comparison URL. |
|
Finished benchmarking try commit 75a5b718ee128eee283eb50bc379c66401add1e5 |
|
Perf results are very similar to what I saw locally: big |
|
@bors r+ awesome! |
|
📌 Commit f2871a9 has been approved by |
bors
added
S-waiting-on-bors
|
Given the impact of the regression this fixes, is it appropriate to give it higher priority? |
|
@bors p=1 |
Make `intern_lazy_const` actually intern its argument. Currently it just unconditionally allocates it in the arena. For a "Clean Check" build of the the `packed-simd` benchmark, this change reduces both the `max-rss` and `faults` counts by 59%; it slightly (~3%) increases the instruction counts but the `wall-time` is unchanged. For the same builds of a few other benchmarks, `max-rss` and `faults` drop by 1--5%, but instruction counts and `wall-time` changes are in the noise. Fixes #57432, fixes #57829.
|
☀️ Test successful - checks-travis, status-appveyor |
|
Perf results from landing are as expected. |
|
Strange, why did max-rss increase by 15% for some benchmarks? |
|
@RalfJung You have to store a map to each constant value, which is wasteful if you only have distinct constant values. Maybe that's why? The max-rss benchmarks are also noisy =P |
|
And |
oli-obk
added
the
beta-nominated
|
nominating for beta backport as per #57829 (comment) We can also create a less invasive backport by just doing the changes in https://github.com/rust-lang/rust/pull/58207/files#diff-c8a6d543f758cb294320bcac3b5268ae without renaming the method |
|
triage, beta-accepted. |
pnkfelix
added
the
beta-accepted
emilyalbini
removed
beta-nominated
[beta] Rollup backports Cherry-picked: * #58207: Make `intern_lazy_const` actually intern its argument. * #58161: Lower constant patterns with ascribed types. * #57908: resolve: Fix span arithmetics in the import conflict error * #57835: typeck: remove leaky nested probe during trait object method resolution * #57885: Avoid committing to autoderef in object method probing * #57646: Fixes text becoming invisible when element targetted Rolled up: * #58522: [BETA] Update cargo r? @ghost
[beta] Rollup backports Cherry-picked: * #58207: Make `intern_lazy_const` actually intern its argument. * #58161: Lower constant patterns with ascribed types. * #57908: resolve: Fix span arithmetics in the import conflict error * #57835: typeck: remove leaky nested probe during trait object method resolution * #57885: Avoid committing to autoderef in object method probing * #57646: Fixes text becoming invisible when element targetted Rolled up: * #58522: [BETA] Update cargo r? @ghost
11 participants
Currently it just unconditionally allocates it in the arena.
For a "Clean Check" build of the the
packed-simdbenchmark, thischange reduces both the
max-rssandfaultscounts by 59%; itslightly (~3%) increases the instruction counts but the
wall-timeisunchanged.
For the same builds of a few other benchmarks,
max-rssandfaultsdrop by 1--5%, but instruction counts and
wall-timechanges are in thenoise.
Fixes #57432, fixes #57829.