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

More interning #5157

Merged
merged 3 commits into from
Mar 9, 2018
Merged

More interning #5157

merged 3 commits into from
Mar 9, 2018

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Mar 9, 2018

This is a forward approach to interning. Specifically Dependency and PackageId store their names as InternedStrings and leave that value interned as long as possible. The alternative is to make a new interned_name function. The advantage of this approach is that a number of places in the code are doing deb.name() == pid.name() and are now using the fast pointer compare instead of the string compare, without the code needing to change. The disadvantage is that lots of places need to call deref with &* to convert to an &str and sum need to use .to_inner() to get a &'static str.

In a test on #4810 (comment)
Before we got to 10000000 ticks in ~48 sec
After we got to 10000000 ticks in ~44 sec

@rust-highfive
Copy link

r? @matklad

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

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 9, 2018

📌 Commit 1fd3496 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Mar 9, 2018

⌛ Testing commit 1fd3496 with merge 5f83bb4...

bors added a commit that referenced this pull request Mar 9, 2018
More interning

This is a forward approach to interning. Specifically `Dependency` and `PackageId` store their names as `InternedString`s and leave that value interned as long as possible. The alternative is to make a new `interned_name` function. The advantage of this approach is that a number of places in the code are doing `deb.name() == pid.name()` and are now using the fast pointer compare instead of the string compare, without the code needing to change. The disadvantage is that lots of places need to call `deref` with `&*` to convert to an `&str` and sum need to use `.to_inner()` to get a `&'static str`.

In a test on #4810 (comment)
Before we got to 10000000 ticks in ~48 sec
After we got to 10000000 ticks in ~44 sec
@bors
Copy link
Collaborator

bors commented Mar 9, 2018

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

@bors bors merged commit 1fd3496 into rust-lang:master Mar 9, 2018
@Eh2406 Eh2406 deleted the more_interning branch March 9, 2018 20:51
@ehuss ehuss added this to the 1.26.0 milestone Feb 6, 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.

6 participants