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.

Sign up for GitHub

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

Jump to bottom

Make intern_lazy_const actually intern its argument. #58207

Merged
merged 1 commit into from
Feb 9, 2019
Merged

Make intern_lazy_const actually intern its argument. #58207

merged 1 commit into from
Feb 9, 2019

Conversation

nnethercote
Copy link
Contributor

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.

@nnethercote
Make intern_lazy_const actually intern its argument.
f2871a9 
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
Copy link
Collaborator

r? @petrochenkov

(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 Feb 6, 2019
@nnethercote
Copy link
Contributor Author

I'm no expert on interning but this appears to do the trick.

r? @oli-obk

cc @RalfJung

@rust-highfive rust-highfive assigned oli-obk and unassigned petrochenkov Feb 6, 2019
@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Feb 6, 2019

⌛ Trying commit f2871a9 with merge 75a5b718ee128eee283eb50bc379c66401add1e5...

@Zoxc
Copy link
Contributor

Zoxc commented Feb 6, 2019

I find it amusing that this removes intern from the name, but actually adds interning.

@bors
Copy link
Contributor

bors commented Feb 6, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@nnethercote
Copy link
Contributor Author

@rust-timer build 75a5b718ee128eee283eb50bc379c66401add1e5

@rust-timer
Copy link
Collaborator

Success: Queued 75a5b718ee128eee283eb50bc379c66401add1e5 with parent 65e647c, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 75a5b718ee128eee283eb50bc379c66401add1e5

@nnethercote
Copy link
Contributor Author

Perf results are very similar to what I saw locally: big max-rss and faults wins, minor instruction count increases, but no obvious corresponding wall-time increases.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 6, 2019

@bors r+

awesome!

@bors
Copy link
Contributor

bors commented Feb 6, 2019

📌 Commit f2871a9 has been approved by oli-obk

@bors 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 Feb 6, 2019
@RalfJung
Copy link
Member

RalfJung commented Feb 9, 2019

Given the impact of the regression this fixes, is it appropriate to give it higher priority?

@Mark-Simulacrum
Copy link
Member

@bors p=1

@bors
Copy link
Contributor

bors commented Feb 9, 2019

⌛ Testing commit f2871a9 with merge d329d46...

bors added a commit that referenced this pull request Feb 9, 2019
@bors
Auto merge of #58207 - nnethercote:intern_lazy_const, r=oli-obk
d329d46 
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.
@bors
Copy link
Contributor

bors commented Feb 9, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing d329d46 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2019
@bors bors merged commit f2871a9 into rust-lang:master Feb 9, 2019
@bors bors mentioned this pull request Feb 9, 2019
Merged
@nnethercote nnethercote deleted the intern_lazy_const branch February 11, 2019 00:42
@nnethercote
Copy link
Contributor Author

Perf results from landing are as expected.

@RalfJung
Copy link
Member

Strange, why did max-rss increase by 15% for some benchmarks?

@Zoxc
Copy link
Contributor

Zoxc commented Feb 11, 2019

@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

@nnethercote
Copy link
Contributor Author

And inflate and keccak are particularly noisy. Earlier runs didn't show a regression for them.

@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 11, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Feb 11, 2019

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

@pnkfelix
Copy link
Member

triage, beta-accepted.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 14, 2019
@pietroalbini pietroalbini mentioned this pull request Feb 17, 2019
Merged
@pietroalbini pietroalbini removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Feb 17, 2019
bors added a commit that referenced this pull request Feb 17, 2019
@bors
Auto merge of #58537 - pietroalbini:beta-backports, r=pietroalbini
1456495 
[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
bors added a commit that referenced this pull request Feb 20, 2019
@bors
Auto merge of #58537 - pietroalbini:beta-backports, r=pietroalbini
1f84156 
[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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone