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.

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

Use Rc instead of Box for interned strings. #50549

Closed
wants to merge 1 commit into from

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented May 8, 2018

We currently heap-allocate each one twice, once for Interner::names and
once for Interner::strings. Using Rc instead means each one is allocated
once and then shared.

This speeds up numerous rustc-perf runs, the best by 4%.

Here are details for the ones with an improvement of 1% or more:

coercions-check
        avg: -2.3%      min: -4.1%      max: -0.8%
coercions
        avg: -1.3%      min: -2.8%      max: -0.8%
coercions-opt
        avg: -1.5%      min: -2.7%      max: -0.6%
tuple-stress
        avg: -0.5%      min: -1.3%      max: 0.1%
tuple-stress-check
        avg: -0.6%      min: -1.3%      max: -0.3%
tuple-stress-opt
        avg: -0.7%      min: -1.3%      max: -0.4%
@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented May 8, 2018

r? @aturon

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

@michaelwoerister
Copy link
Contributor

@michaelwoerister michaelwoerister commented May 9, 2018

We currently heap-allocate each one twice

:D

@Zoxc, do you see a problem with using Rc here? The ref-count should never be changed from different threads, as far as I can tell.

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented May 9, 2018

This use of Rc is fine. It will need an unsafe impl Send for Interner {} though. Add a note that the impl is safe because the ref count is only modified at one thread at a time. It is modified during insertion and during destruction. These may happen on different threads, but are mutually excluded.

@michaelwoerister
Copy link
Contributor

@michaelwoerister michaelwoerister commented May 9, 2018

OTOH, there wouldn't be a real downside to using Lrc, right?

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented May 9, 2018

It would be slower.

@michaelwoerister
Copy link
Contributor

@michaelwoerister michaelwoerister commented May 9, 2018

Yes, but I wouldn't expect the difference to be noticeable.

We currently heap-allocate each one twice, once for Interner::names and
once for Interner::strings. Using Rc instead means each one is allocated
once and then shared.

This speeds up numerous rustc-perf runs, the best by 4%.
@nnethercote nnethercote force-pushed the nnethercote:Rc-interner branch from 355577e to fc202c5 May 9, 2018
@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented May 9, 2018

Updated with the unsafe impl Send.

r? @Zoxc

@rust-highfive rust-highfive assigned Zoxc and unassigned aturon May 9, 2018
bors added a commit that referenced this pull request May 10, 2018
Allocate Symbol strings from an arena

This is an alternative to #50549

cc @nnethercote

r? @michaelwoerister
@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented May 11, 2018

#50607 is a much better way of doing things.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 12, 2018
Allocate Symbol strings from an arena

This is an alternative to rust-lang#50549

cc @nnethercote

r? @michaelwoerister
@nnethercote nnethercote deleted the nnethercote:Rc-interner branch Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.