Intern SourceId #6342

dwijnand · 2018-11-21T20:31:03Z

Refs #6207
sibling of #6332

rust-highfive · 2018-11-21T20:31:14Z

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

alexcrichton · 2018-11-21T21:52:16Z

Aha fascinating! It looks like this is actually working despite my comments about this probably not going to work. I guess the weird hashing I mentioned is only on PackageId and not on SourceId? Want to confirm that and then I can r+?

dwijnand · 2018-11-21T22:24:38Z

Nope, PackageId has a straightforward hashing definition. It also has a stable_hash function that is defined in terms of SourceId's stable_hash.

SourceId has two weird hashing implementations impl Hash for SourceId and stable_hash, and they're different from one another. 😳

Why CI passes here, I don't know.

Eh2406 · 2018-11-22T00:27:27Z

The CI failure is spurious. I don't seam to be able to force a retry.

Eh2406 · 2018-11-22T21:13:57Z

Two small things and a big one.

SourceId can now derive(Copy), allowing us to remove the .clone() calls. (We can leave the clean up for a separate PR, like the next time we try to get Clippy happy, if you want.)
impl PartialEq for SourceId can "just" be a pointer comparison. If they where eq the interning would have pointed to the same thing. (Saves 2 cache miss for each time we call it.)

and the big one:

impl PartialEq for SourceIdInner I think the interning will de-duplicate using this algorithm. This means that SourceId::new(x, url).url == url may no longer be true. (@alexcrichton Is this ok?) If that is ok, we should document it, else we will have to figure out how to get the interning to keep track of it. In ether case we should add a test.

@dwijnand, I did not know haw many fiddly details this quest issue was going to hit, sorry.

dwijnand · 2018-11-22T23:32:00Z

This means that SourceId::new(x, url).url == url may no longer be true. (@alexcrichton Is this ok?)

I think if PartialEq is defined like that, that must be acceptable.

dwijnand · 2018-11-23T16:34:51Z

Restarting AppVeyor to clear the spurious failure, the pleb way.

dwijnand · 2018-11-23T21:31:49Z

And it's green. Just like magic.

alexcrichton · 2018-11-25T09:47:24Z

Hm ok so looking into this, I'm remembering a bit about what's going on here. While there's a bunch of fields of a SourceId they're basically all ignored for hashing/equality except the kind (path/git/registry/...) and the canonical URL (dealing with github, slashes, etc). This means that #[derive(Hash)] for an inner source id is very different from the outer Hash for SourceId. It also means that we can't switch to pointer equality to PartialEq just yet because two equal SourceId instances may have different SourceIdInner instances.

This is a little subtle at this point, and perhaps worth a comment as well? I think our test coverage is good enough that I'm not too worried about landing it though!

dwijnand · 2018-11-25T10:05:17Z

Definitely sounds like it's worth a comment. Coming right up.

dwijnand · 2018-11-25T10:54:38Z

After studying your comment and the in-repo code and comments I think SourceIdInner doesn't obey the k1 == k2 -> hash(k1) == hash(k2) law. I find that a bit concerning. Do you think we should address that?

(edit: I introduced the unlawfulness, fixed below)

dwijnand · 2018-11-25T12:00:34Z

To recap this is the current situation, in this PR:

PartialEq for SourceId is defined in terms of SourceIdInner equality
Hash for SourceId is kind hash + canonical_url (for git) or url hash
PartialEq for SourceIdInner is canonical_url (for git) or url equality, ignoring the precise and name fields
Hash for SourceIdInner is auto-derived, which makes SourceIdInner unlawful

It also means that we can't switch to pointer equality to PartialEq just yet because two equal SourceId instances may have different SourceIdInner instances.

I think it's true we can't switch to pointer equality, but for the opposite reason: two SourceId instances might be pointing to slightly different SourceIdInner, but which are considered equal in our custom equality definition.

dwijnand · 2018-11-25T12:12:29Z

OK I think I know how to fix this: we move the custom equality up to SourceId and let SourceIdInner auto-derive its fully equality. That way we:

preserve that SourceIds that have slightly different URLs are actually equal
make SourceIdInner lawful in its Hash/Eq definition.

dwijnand · 2018-11-25T12:26:11Z

Just realised I was the one that made SourceIdInner auto-derive Hash, thus making it unlawful 😆

That way we: * preserve that SourceIds that have slightly different URLs are actually equal * make SourceIdInner lawful in its Hash/Eq definition.

Eh2406 · 2018-11-25T14:59:09Z

Looks correct to me. Alex thinks the existing tests are sufficient, and the CI is green.

@bors: r+

bors · 2018-11-25T14:59:10Z

📌 Commit 9b22eb1 has been approved by Eh2406

bors · 2018-11-25T14:59:17Z

⌛ Testing commit 9b22eb1 with merge 1ff5975...

Intern SourceId Refs #6207 sibling of #6332

bors · 2018-11-25T15:23:15Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Eh2406
Pushing 1ff5975 to master...

remove clones made redundant by Intern SourceId This is a follow up to #6342. I used clippy to find all the places we called `.clone()` on a `SourceId` or where we passed `&SourceId`. This also involved fixing a large number of other things clippy was complaining about, and running `fmt` on a large number of files.

Update cargo 14 commits in b3d0b2e545b61d4cd08096911724b7d49d213f73..1ff5975b96b3d395bb962394596998dfb485f793 2018-11-15 19:13:04 +0000 to 2018-11-25 14:59:12 +0000 - Intern SourceId (rust-lang/cargo#6342) - Tweak Layout to allow for non json file targets with internal "." (rust-lang/cargo#6255) - Correct Target Directory command-line option (rust-lang/cargo#6343) - Persistent data structures by im-rs (rust-lang/cargo#6336) - Move command prelude into main library (rust-lang/cargo#6335) - Distinguish custom build invocations (rust-lang/cargo#6331) - Allow crate_type=bin examples to run (rust-lang/cargo#6330) - Make verify-project honour unstable features (rust-lang/cargo#6326) - Make autodiscovery disable inferred targets (rust-lang/cargo#6329) - Add `c` alias for `check` (rust-lang/cargo#6218) - Allow user aliases to override built-in aliases (rust-lang/cargo#6259) - Fix renaming directory project using build scripts with cross-compiling. (rust-lang/cargo#6328) - Fix add_plugin_deps-related tests. (rust-lang/cargo#6327) - Add a glossary. (rust-lang/cargo#6321)

Update cargo, rls 26 commits in b3d0b2e545b61d4cd08096911724b7d49d213f73..5e85ba14aaa20f8133863373404cb0af69eeef2c 2018-11-15 19:13:04 +0000 to 2018-12-02 14:37:25 +0000 - ConflictStoreTrie: Faster filtered search (rust-lang/cargo#6366) - Remove `cmake` as a requirement (rust-lang/cargo#6368) - progress: display "Downloading 1 crate" instead of "Downloading 1 crates" (rust-lang/cargo#6369) - Use expect over unwrap, for panic-in-panic aborts (rust-lang/cargo#6364) - Switch to pretty_env_logger, under --features pretty-env-logger (rust-lang/cargo#6362) - use allow-dirty option in `cargo package` to skip vcs checks (rust-lang/cargo#6280) - remove clones made redundant by Intern PackageId (rust-lang/cargo#6352) - docs: correct profile usage of `cargo test --release` (rust-lang/cargo#6345) - Improve doc for `cargo install` (rust-lang/cargo#6354) - Intern PackageId (rust-lang/cargo#6332) - Clean only release artifacts if --release option is set (rust-lang/cargo#6349) - remove clones made redundant by Intern SourceId (rust-lang/cargo#6347) - Intern SourceId (rust-lang/cargo#6342) - Tweak Layout to allow for non json file targets with internal "." (rust-lang/cargo#6255) - Correct Target Directory command-line option (rust-lang/cargo#6343) - Persistent data structures by im-rs (rust-lang/cargo#6336) - Move command prelude into main library (rust-lang/cargo#6335) - Distinguish custom build invocations (rust-lang/cargo#6331) - Allow crate_type=bin examples to run (rust-lang/cargo#6330) - Make verify-project honour unstable features (rust-lang/cargo#6326) - Make autodiscovery disable inferred targets (rust-lang/cargo#6329) - Add `c` alias for `check` (rust-lang/cargo#6218) - Allow user aliases to override built-in aliases (rust-lang/cargo#6259) - Fix renaming directory project using build scripts with cross-compiling. (rust-lang/cargo#6328) - Fix add_plugin_deps-related tests. (rust-lang/cargo#6327) - Add a glossary. (rust-lang/cargo#6321)

Intern SourceId

3d91b79

rust-highfive assigned alexcrichton Nov 21, 2018

dwijnand closed this Nov 23, 2018

dwijnand reopened this Nov 23, 2018

dwijnand added 3 commits November 25, 2018 12:31

Move the custom equality up to SourceId

653ce4d

That way we: * preserve that SourceIds that have slightly different URLs are actually equal * make SourceIdInner lawful in its Hash/Eq definition.

Add pointer equality to PartialEq for SourceId

ba175dc

Make SourceId derive Copy

9b22eb1

dwijnand force-pushed the intern-SourceId branch from 3cb173e to 9b22eb1 Compare November 25, 2018 13:44

bors added a commit that referenced this pull request Nov 25, 2018

Auto merge of #6342 - dwijnand:intern-SourceId, r=Eh2406

1ff5975

Intern SourceId Refs #6207 sibling of #6332

bors merged commit 9b22eb1 into rust-lang:master Nov 25, 2018

dwijnand deleted the intern-SourceId branch November 25, 2018 15:24

Eh2406 mentioned this pull request Nov 25, 2018

remove clones made redundant by Intern SourceId #6347

Merged

ehuss mentioned this pull request Nov 25, 2018

Update cargo, rls rust-lang/rust#56224

Merged

Eh2406 mentioned this pull request Sep 17, 2019

Why so many lifetimes? #7342

Closed

ehuss added this to the 1.32.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intern SourceId #6342

Intern SourceId #6342

dwijnand commented Nov 21, 2018

rust-highfive commented Nov 21, 2018

alexcrichton commented Nov 21, 2018

dwijnand commented Nov 21, 2018

Eh2406 commented Nov 22, 2018

Eh2406 commented Nov 22, 2018

dwijnand commented Nov 22, 2018

dwijnand commented Nov 23, 2018

dwijnand commented Nov 23, 2018

alexcrichton commented Nov 25, 2018

dwijnand commented Nov 25, 2018

dwijnand commented Nov 25, 2018 •

edited

Loading

dwijnand commented Nov 25, 2018 •

edited

Loading

dwijnand commented Nov 25, 2018

dwijnand commented Nov 25, 2018 •

edited

Loading

Eh2406 commented Nov 25, 2018

bors commented Nov 25, 2018

bors commented Nov 25, 2018

bors commented Nov 25, 2018

Intern SourceId #6342

Intern SourceId #6342

Conversation

dwijnand commented Nov 21, 2018

rust-highfive commented Nov 21, 2018

alexcrichton commented Nov 21, 2018

dwijnand commented Nov 21, 2018

Eh2406 commented Nov 22, 2018

Eh2406 commented Nov 22, 2018

dwijnand commented Nov 22, 2018

dwijnand commented Nov 23, 2018

dwijnand commented Nov 23, 2018

alexcrichton commented Nov 25, 2018

dwijnand commented Nov 25, 2018

dwijnand commented Nov 25, 2018 • edited Loading

dwijnand commented Nov 25, 2018 • edited Loading

dwijnand commented Nov 25, 2018

dwijnand commented Nov 25, 2018 • edited Loading

Eh2406 commented Nov 25, 2018

bors commented Nov 25, 2018

bors commented Nov 25, 2018

bors commented Nov 25, 2018

dwijnand commented Nov 25, 2018 •

edited

Loading

dwijnand commented Nov 25, 2018 •

edited

Loading

dwijnand commented Nov 25, 2018 •

edited

Loading