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

Tweak `Symbol` and `InternedString` #60659

Merged
merged 5 commits into from May 10, 2019

Conversation

Projects
None yet
6 participants
@nnethercote
Copy link
Contributor

commented May 9, 2019

Some minor improvements to speed and code cleanliness.

r? @Zoxc

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

⌛️ Trying commit 3e5e8d3 with merge 9a6dab6...

bors added a commit that referenced this pull request May 9, 2019

Auto merge of #60659 - nnethercote:tweak-Symbol-and-InternedString, r…
…=<try>

Tweak `Symbol` and `InternedString`

Some minor improvements to speed and code cleanliness.

r? @Zoxc
@bors

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

☀️ Try build successful - checks-travis
Build commit: 9a6dab6

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 9, 2019

Success: Queued 9a6dab6 with parent 9f83961, comparison URL.

@petrochenkov petrochenkov self-assigned this May 9, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I got miniscule improvements locally, but I think it's worthwhile from the clean-up angle.

//
// FIXME: ensure that the interner outlives any thread which uses
// `LocalInternedString`, by creating a new thread right after constructing the
// interner.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov May 9, 2019

Contributor

@Zoxc , I don't think this FIXME is necessary?

This comment has been minimized.

Copy link
@Zoxc

Zoxc May 10, 2019

Contributor

Why not?

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov May 10, 2019

Contributor

I assumed this is already true due to how GLOBALS and threads are created.

This comment has been minimized.

Copy link
@Zoxc

Zoxc May 10, 2019

Contributor

I think it happens do be done that way now, but it's not enforced or documented as required by symbol.rs.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Could you revert the fn get -> fn symbol_str/fn symbol_or_gensym_str change?
It's ideologically wrong and it's not faster since symbol_str is still doing checked indexing.

@rust-timer

This comment has been minimized.

Copy link

commented May 9, 2019

Finished benchmarking try commit 9a6dab6

nnethercote added some commits May 9, 2019

Add `InternedString::with2`.
This lets comparisons occur with a single access to the interner,
instead of two.
Add various comments.
Lots of details I wish I'd known when I first looked at this code.

@nnethercote nnethercote force-pushed the nnethercote:tweak-Symbol-and-InternedString branch from 3e5e8d3 to e53bb1a May 10, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@petrochenkov: New code is up.

@@ -367,10 +380,6 @@ impl Symbol {
with_interner(|interner| interner.intern(string))
}

pub fn interned(self) -> Self {

This comment has been minimized.

Copy link
@Zoxc

Zoxc May 10, 2019

Contributor

Why is this removed?

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov May 10, 2019

Contributor

No longer used.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

r=me is @Zoxc doesn't have more concerns

r? @Zoxc

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

📌 Commit e53bb1a has been approved by Zoxc

Centril added a commit to Centril/rust that referenced this pull request May 10, 2019

Rollup merge of rust-lang#60659 - nnethercote:tweak-Symbol-and-Intern…
…edString, r=Zoxc

Tweak `Symbol` and `InternedString`

Some minor improvements to speed and code cleanliness.

r? @Zoxc

bors added a commit that referenced this pull request May 10, 2019

Auto merge of #60708 - Centril:rollup-j5smdo0, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #60529 (RFC 2008: Uninhabitedness fixes for enum variants and tests)
 - #60620 (Fix a couple of FIXMEs in ext::tt::transcribe)
 - #60659 (Tweak `Symbol` and `InternedString`)
 - #60692 (Extend #60676 test for nested mut patterns.)
 - #60697 (add regression test for #60629)
 - #60701 (Update mailmap for mati865)

Failed merges:

r? @ghost

@bors bors merged commit e53bb1a into rust-lang:master May 10, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@nnethercote nnethercote deleted the nnethercote:tweak-Symbol-and-InternedString branch May 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.