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

Change the hashcounts in raw Lit variants from usize to u16. #49993

Merged
merged 1 commit into from
Apr 18, 2018

Conversation

nnethercote
Copy link
Contributor

This reduces the size of Token from 32 bytes to 24 bytes on 64-bit
platforms.

This reduces the size of `Token` from 32 bytes to 24 bytes on 64-bit
platforms.
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Apr 16, 2018
@michaelwoerister
Copy link
Member

Thanks, @nnethercote!

Re-assigning to @alexcrichton, since it touches libproc_macro.

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 16, 2018

📌 Commit 4d34bfd has been approved by alexcrichton

@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 Apr 16, 2018
@Mark-Simulacrum
Copy link
Member

Out of interest, it feels like this could go lower -- I can't imagine a valid use case for more than 256 of these; did you investigate whether that would net further savings?

@nnethercote
Copy link
Contributor Author

Lit now has a 1 byte tag, a 2 byte hash count for the StrRaw and ByteStrRaw variants, and a 4 byte ast::Name, all of which fits into 8 bytes along with one byte of padding. Reducing the hash count to 1 byte wouldn't change things, because it would be replaced by an extra byte of padding.

We could get the size of Token down from 24 bytes to 16 bytes with some more effort, by shrinking the Literal variant. The Option<ast::Name> currently takes up 8 bytes, which is wasteful. I fiddled with making ast::Name use NonZeroU32, but it was a bit of a hassle. Another possibility is to rearrange the Literal/Lit nesting, which I also deemed not worth the effort, because the measurable benefit of shrinking from 32 to 24 bytes was negligible. Others might have more luck.

@bors
Copy link
Contributor

bors commented Apr 18, 2018

⌛ Testing commit 4d34bfd with merge 3dfda16...

bors added a commit that referenced this pull request Apr 18, 2018
Change the hashcounts in raw `Lit` variants from usize to u16.

This reduces the size of `Token` from 32 bytes to 24 bytes on 64-bit
platforms.
@bors
Copy link
Contributor

bors commented Apr 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 3dfda16 to master...

@bors bors merged commit 4d34bfd into rust-lang:master Apr 18, 2018
@nnethercote nnethercote deleted the shrink-Token branch April 18, 2018 22:24
@eddyb
Copy link
Member

eddyb commented Apr 25, 2018

Oh, btw, the Option around the suffix in Literal could be removed (and instead you can check that it's equal to the empty Symbol, instead of None).

@nnethercote
Copy link
Contributor Author

I did a rustc-perf run for this. It was a measurable win for some of the coercions and html5ever jobs:

coercions
        avg: -0.5%      min: -1.7%      max: 0.1%
html5ever-opt
        avg: -0.7%      min: -1.3%      max: -0.3%
html5ever-check
        avg: -0.7%      min: -1.2%      max: -0.3%
coercions-opt
        avg: -0.3%      min: -1.0%      max: -0.1%

@nnethercote
Copy link
Contributor Author

Oh, btw, the Option around the suffix in Literal could be removed (and instead you can check that it's equal to the empty Symbol, instead of None).

The downside is that this makes Literal harder to work with -- you can't pattern match easily the way you can with Option, and creating an empty suffix involves a call to intern().

@eddyb
Copy link
Member

eddyb commented Apr 30, 2018

@nnethercote Note that almost nobody should be touching literals though that internal enum, only type-checking and MIR construction. Also, the empty string is preinterned:

(0, Invalid, "")

(although "Invalid" is not the best name for it - cc @petrochenkov @jseyfried)

@nnethercote
Copy link
Contributor Author

The preinterning of the empty string helps; thank you for pointing that out.

But overall it's not worth getting rid of the Option<>. I tried it and got it working, but the code becomes uglier, and it's a very small slowdown -- the time saved by making Token smaller is slightly outweighed by slower "is there a suffix?" checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants