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 `Symbol` even more #60815

Merged
merged 3 commits into from May 20, 2019

Conversation

Projects
None yet
6 participants
@nnethercote
Copy link
Contributor

commented May 14, 2019

These patches simplify the code a bit (fewer conversions) and also speed things up a bit (fewer with_interner calls).

r? @petrochenkov

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I realize that this will need rebasing once #60740 lands.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

⌛️ Trying commit 67b969e with merge 8e849d8...

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

Auto merge of #60815 - nnethercote:use-Symbol-more-2, r=<try>
Use `Symbol` even more

These patches simplify the code a bit (fewer conversions) and also speed things up a bit (fewer `with_interner` calls).

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

☀️ Try build successful - checks-travis
Build commit: 8e849d8

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 14, 2019

Success: Queued 8e849d8 with parent 80e7cde, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented May 14, 2019

Finished benchmarking try commit 8e849d8

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Any data structure using InternedString may use it for a reason, due to stable hashing.
IIRC, that's exactly the case for generic parameters, but I'll leave this to @michaelwoerister to review.

The (Local)InternedString::intern change LGTM.

r? @michaelwoerister

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@petrochenkov, do we still use gensym? IIRC, we pushed for using InternedString in all cases where it was really only the raw string data that we care about. With "gensymmed" Symbols it's possible to distinguish between two identifiers with the same string contents, be there's no way to implement HashStable for that.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Gensyms are still used.

Git blame says that InternedString in generic parameters was introduced in #49695 to fix #48923.
It looks like there's no specific regression test for that issue, so nothing in the test suite regressed from this PR.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

It looks like there's no specific regression test for that issue

Sigh. My view is that Symbol is the fundamental type, and InternedString and LocalInternedString are alternatives that should only be used when necessary. (Using Symbol as much as possible is also good for performance.) I've been using the test suite as the guide for what conversions are possible, but if the test suite isn't comprehensive that makes things difficult :(

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

More generally: the difference between Symbol and InternedString is that the latter does PartialOrd/Ord and Hash on the chars, rather than the index.

  • For PartialOrd/Ord I imagine the difference might affect some error messages.
  • For Hash I am surprised that it is important, because if you need a stable hash you should use HashStable anyway, and Symbol could easily implement HashStable on the chars.
@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Apparently the issue in #48923 was caused by query infrastructure using both 1) StableHash and 2) Hash/PartialEq.
If two things have the same StableHash, but different Hash/PartialEq then things break (that's exactly what happens with gensyms).

I don't know whether it's possible to use only StableHash and some different equality comparison function or trait instead of Hash and PartialEq in the query infrastructure.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Finally, I had to work out the difference between Symbol and InternedString myself, because there was no explanation in the code. (There is a comment about it now because I added one.) And none of the places that use InternedString have any explanation why they use it instead of Symbol, as far as I can tell. It is frustrating.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Yes, IIRC, I was not able to come up with a regression test for the fix in #49695. Adding an assertion that would catch the error early was the best I was able to do at the time:

// If the following assertion triggers, it can have two reasons:
// 1. Something is wrong with DepNode creation, either here or
// in DepGraph::try_mark_green()
// 2. Two distinct query keys get mapped to the same DepNode
// (see for example #48923)
assert!(!self.dep_graph.dep_node_exists(&dep_node),
"Forcing query with already existing DepNode.\n\
- query-key: {:?}\n\
- dep-node: {:?}",
key, dep_node);

@nnethercote nnethercote force-pushed the nnethercote:use-Symbol-more-2 branch from 67b969e to 2175494 May 17, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

I have pushed new code that reverts the InternedString-to-Symbol change for ParamTy and related types. Some (but not all) of the performance benefit is lost as a result.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

⌛️ Trying commit 2175494 with merge ac67417...

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

Auto merge of #60815 - nnethercote:use-Symbol-more-2, r=<try>
Use `Symbol` even more

These patches simplify the code a bit (fewer conversions) and also speed things up a bit (fewer `with_interner` calls).

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

☀️ Try build successful - checks-travis
Build commit: ac67417

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 17, 2019

Success: Queued ac67417 with parent 4f53b5c, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented May 17, 2019

Finished benchmarking try commit ac67417: comparison url

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Still a clear perf improvement with the new code. It would be good to land this.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

📌 Commit 2175494 has been approved by petrochenkov

nnethercote added some commits May 14, 2019

Avoid `as_str()` in `ParamTy::is_self`.
It's a hot function, and a direct `Symbol` comparison is faster.

The patch also converts some `&InternedString`s to `InternedString`.
Introduce `InternedString::intern`.
`InternedString::intern(x)` is preferable to
`Symbol::intern(x).as_interned_str()`, because the former involves one
call to `with_interner` while the latter involves two.

The case within InternedString::decode() is particularly hot, and this
change reduces the number of `with_interner` calls by up to 13%.
Introduce `LocalInternedString::intern`.
`LocalInternedString::intern(x)` is preferable to
`Symbol::intern(x).as_str()`, because the former involves one call to
`with_interner` while the latter involves two.

@nnethercote nnethercote force-pushed the nnethercote:use-Symbol-more-2 branch from 2175494 to c06cdbe May 19, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

I rebased.

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

📌 Commit c06cdbe has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

⌛️ Testing commit c06cdbe with merge caef1e8...

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

Auto merge of #60815 - nnethercote:use-Symbol-more-2, r=petrochenkov
Use `Symbol` even more

These patches simplify the code a bit (fewer conversions) and also speed things up a bit (fewer `with_interner` calls).

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing caef1e8 to master...

@bors bors added the merged-by-bors label May 20, 2019

@bors bors merged commit c06cdbe into rust-lang:master May 20, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:use-Symbol-more-2 branch May 21, 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.