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

Shrink token::Lit's hash count from usize to u16. #38043

Closed
wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

On 64-bit platforms, this reduces the size of token::Lit from 16 bytes to 8 bytes, and reduces the size of token::Token from 32 bytes to 24 bytes.

There are two places where I could add overflow checks, and I've marked them with "njn" comments. It's unlikely that there would be more than 65535 '#' marks at the start of a raw string, but I can add a check if you think it's worthwhile.

r? @eddyb

On 64-bit platforms, this reduces the size of `token::Lit` from 16 bytes
to 8 bytes, and reduces the size of `token::Token` from 32 bytes to 24
bytes.
@KalitaAlexey
Copy link
Contributor

What is your motivation? What will it give? How much memory it will save?

@nnethercote
Copy link
Contributor Author

It's more about reducing memory traffic. Fewer bytes written, read, copied, etc.

@bors
Copy link
Contributor

bors commented Dec 4, 2016

☔ The latest upstream changes (presumably #38148) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Copy link
Contributor Author

@eddyb: I think the compiler now auto-sorts fields to minimize struct size. Is that right? If so, this change shouldn't be necessary.

@sanxiyn
Copy link
Member

sanxiyn commented Jan 1, 2017

We decided to revert field reordering for now, see #38523.

@nnethercote
Copy link
Contributor Author

Field reordering seems very likely to be turned on at some point, so let's close this.

@nnethercote nnethercote closed this Jan 2, 2017
@nnethercote nnethercote deleted the shrink-Lit branch January 2, 2017 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants