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 a flat layout for TokenBuffer #1223

Merged
merged 1 commit into from
Sep 28, 2022
Merged

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Sep 28, 2022

This looks like a small perf win:

flat-tokenbuffer

test baseline            ... bench:     751,638 ns/iter (+/- 37,662)   [0.714ms..0.789ms]
test create_token_buffer ... bench:   2,624,105 ns/iter (+/- 218,503)  [2.416ms..2.843ms]
test parse_file          ... bench:   6,546,490 ns/iter (+/- 285,880)  [6.261ms..6.832ms]

main

test baseline            ... bench:     766,913 ns/iter (+/- 35,923)   [0.731ms..0.802ms]
test create_token_buffer ... bench:   2,888,407 ns/iter (+/- 167,829)  [2.721ms..3.056ms]
test parse_file          ... bench:   6,737,680 ns/iter (+/- 492,060)  [6.316ms..7.160ms]

Replaces #1196. With a flat tokenbuffer, implementing Ord for Cursor will become trivial.

I did try a bit to make Cursor store &[Entry] directly, but I was having trouble doing so as part of this change. It might still be a worthwhile followup to reduce the necessary amount of unsafe, but doesn't need to be done immediately.

The biggest code quality question is probably how I handled changing bump.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you.

@dtolnay dtolnay merged commit 91f0141 into dtolnay:master Sep 28, 2022
@CAD97 CAD97 deleted the flat-tokenbuffer branch September 29, 2022 01:39
Comment on lines +43 to 44
// Keep the explicit impl for backwards compatibility.
impl Drop for TokenBuffer {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to figure out the purpose of this. Is this to support code that did fn f<T: Drop> and called f::<TokenBuffer>? If so, that's "you brought this upon yourself" territory and I wouldn't care about breaking it. Is there any plausible practical code that would need this impl to exist?

Or is it the reverse concern, that there might be code that breaks if we later need to add back a Drop impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's paranoia around letter-of-law and the former — I don't think there's any other impact, since there aren't any generics to capture potentially late bound lifetimes in.

dtolnay added a commit that referenced this pull request Sep 29, 2022
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.

None yet

2 participants