Store fewer NodeIds in crate metadata. by michaelwoerister · Pull Request #43887 · rust-lang/rust

michaelwoerister · 2017-08-15T15:57:46Z

The first commit just replaces NodeId with DefId in resolve_lifetime::Region, so we don't run into problems when stable-hashing TypeParameterDef values from other crates.

The second commit is more interesting. It adds the ReScopeAnon variant to ty::RegionKind, which is semantically equivalent to ReScope. The only difference is that it does not contain a CodeExtent field. All ReScope occurrences are then replaced with ReScopeAnon upon metadata export. This way we don't end up with NodeIds from other crates in various things imported from metadata. ReScopeAnon can still be tested for equality. This is fixed in a better way by @eddyb in #44171.

r? @eddyb
cc @rust-lang/compiler

eddyb · 2017-08-15T16:39:26Z

I agree with the resolve_lifetime change, in fact it's probably past time we moved to ty::RegionKind in that pass, I forget why I couldn't make that change fully.

The ReScopeAnon change OTOH... Can't we just transition CodeExtent to the new IDs?
That is, ReScope is only valid within a body (and all closure bodies contained within), so keeping only the intra-item ID would be the right thing to do - I believe I've pitched this to @nikomatsakis in the past and he agreed. RegionMaps are per-item anyway now so nothing should conflict.

kennytm · 2017-08-15T17:10:31Z

Cannot build rustdoc.

[00:28:55] error[E0308]: mismatched types
[00:28:55] --> /checkout/src/librustdoc/clean/mod.rs:834:61
[00:28:55] |
[00:28:55] 834 | if let Some(lt) = cx.lt_substs.borrow().get(&node_id).cloned() {
[00:28:55] | ^^^^^^^^ expected struct `syntax::ast::NodeId`, found struct `rustc::hir::def_id::DefId`
[00:28:55] |
[00:28:55] = note: expected type `&syntax::ast::NodeId`
[00:28:55] found type `&rustc::hir::def_id::DefId`
[00:28:55] [00:28:57] error: aborting due to previous error
[00:28:57] [00:28:57] error: Could not compile `rustdoc`.

bors · 2017-08-16T06:46:56Z

☔ The latest upstream changes (presumably #43710) made this pull request unmergeable. Please resolve the merge conflicts.

michaelwoerister · 2017-08-16T09:19:19Z

Can't we just transition CodeExtent to the new IDs?

I'm fine with that if we can really switch it to ItemLocalId -- switching to HirId won't help because we again don't know which crate the DefIndex in there belongs to.

However, it's also a question of whether CodeExtent will still be a thing when we completely switch to MIR borrow check. I've asked a few times on IRC but nobody responded. If CodeExtent were to go away, I would not want to spend time on refactoring it now.

eddyb · 2017-08-16T09:27:21Z

@michaelwoerister The MIR borrow-checker will likely use a different representation, but I expect it to be months away. If CodeExtent causes problems now, a transition to ItemLocalId should work.

michaelwoerister · 2017-08-16T09:46:46Z

I'm not convinced yet. This is not a straightforward refactoring. Making CodeExtent store only an ItemLocalId would lose information that might not always be reconstructable from the context and it would open the door for accidentally mixing ItemLocalIds from different owners. I would not feel comfortable doing this change without putting runtime checks into place and being very careful. For the TypeckTables refactoring it took several days to do this.

What I could also do is using (CrateNum, HirId) as identifier but that seemed less attractive than the anonymization, which is a clean workaround.

arielb1 · 2017-08-16T11:31:24Z

@michaelwoerister

I think the plan with NLL (not MIR borrow-check) is to do away with ReScope and to have an "implicit" map between region variables and concrete regions.

nikomatsakis · 2017-08-17T18:56:14Z

Some thoughts:

First off, I agree that as we move to NLL, this stuff will change and in particular ReScope will go away. However, clearly we need to make progress on incremental in the meantime.

My first reaction was similar to @eddyb -- that is, I would prefer to move away from NodeId to something like LocalId, and not have to do any kind of "rewriting" when generated the metadata. However, I also understand @michaelwoerister's concern about surprise interactions from such a change, particularly around closure interfaces.

Presumably we could rewrite CodeExtent to use not just a NodeId but one that carries a crate, right? It might increase the size of RegionKind -- but I think it wouldn't because of the ReEarlyBound(EarlyBoundRegion) variant. Is there another reason this is obviously a bad idea?

Alternatively, I think the approach that @michaelwoerister wound up with might be ok "for a while", since we're planning on removing or reworking ReScope eventually.

michaelwoerister · 2017-08-17T19:26:23Z

Moving to LocalId is not an easy refactoring, I think. I estimate that it would take at least a week to do cleanly (which means it would actually take at least two :P) Given that the whole thing will go away in the medium term, this does not seem like time well spent.

Using (CrateNum, HirId) or something in that vain would be trivial. My concerns would indeed be increased memory usage and a vague uneasiness about copying this almost useless data around. It might not actually make a difference in practice though.

eddyb · 2017-08-18T02:37:20Z

I'm surprised CodeExtent is "global" in any sense anymore. I don't remember where that would show up.

alexcrichton · 2017-08-24T14:22:52Z

ping @michaelwoerister, just wanted to make sure this didn't fall off your radar!

michaelwoerister · 2017-08-24T15:59:45Z

Well, my proposed solution to the ReScope problem is implemented in the PR as it is. I still think it is the least disruptive one and none of the alternatives seems to have gotten much traction, so I'm not quite sure how to proceed.

nikomatsakis · 2017-08-24T19:51:44Z

@eddyb

I'm surprised CodeExtent is "global" in any sense anymore. I don't remember where that would show up.

It could show up in closure signatures, I believe. I'm not sure where else (hopefully not many places, if any!)

eddyb · 2017-08-24T19:53:54Z

@nikomatsakis I'm considering closures to be sharing ItemLocalId spaces anyway.

carols10cents · 2017-08-28T14:16:56Z

@nikomatsakis @eddyb what are the next steps here? how should @michaelwoerister proceed?

eddyb · 2017-08-28T14:21:26Z

I still think CodeExtent can use ItemLocalId without worries.

bors · 2017-08-28T19:27:25Z

☔ The latest upstream changes (presumably #43076) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2017-08-28T21:13:03Z

I'm not sure how to reach consensus on this. I guess I feel somewhere in between @eddyb and @michaelwoerister -- I feel like it's probably true that it would work, but I don't know how best to be sure, or how to test if I am wrong other than weird ICEs. I'd prefer to unblock @michaelwoerister, so I am inclined in favor of the implementation that is actually working, but given that he is not around this week, maybe I will try to some time to read into the code a bit more to convince myself a bit more about it.

eddyb · 2017-08-29T04:37:42Z

I'll attempt the ItemLocalId in CodeExtent today and report back if I notice anything suspicious along the way.

eddyb · 2017-08-29T11:25:22Z

I think the only problem so far is error reporting for lifetimes which is a mess. I'll try to see what I can do about that.

eddyb · 2017-08-31T09:48:27Z

@michaelwoerister I was wondering if you had any tests for this change, to check them on top of #44171 - but I don't see any in this PR.

michaelwoerister · 2017-09-04T15:04:40Z

@bors retry

Looks like some kind of timeout...

bors · 2017-09-04T15:20:35Z

⌛ Testing commit 773736c with merge 90e3d4253f5148dac644a36798742c1b443a2b5d...

bors · 2017-09-04T16:14:20Z

💔 Test failed - status-travis

kennytm · 2017-09-04T17:50:45Z

@bors retry the macs are timing out again.

bors · 2017-09-04T17:56:16Z

⌛ Testing commit 773736c with merge de9d61169be4b587fc069de2adedb089bb6ae71a...

bors · 2017-09-04T18:36:12Z

💔 Test failed - status-travis

kennytm · 2017-09-04T19:05:07Z

@bors retry p=-1 #44221

(reduce priority for third try)

bors · 2017-09-05T12:36:21Z

⌛ Testing commit 773736c with merge f49707b9f4ecb7ba964db2c1509d696183500ea9...

bors · 2017-09-05T13:58:23Z

💔 Test failed - status-appveyor

kennytm · 2017-09-05T14:14:08Z

@bors retry p=0 #43402

`check x86_64-pc-windows-gnu` failed 😒

[01:19:10] failures:
[01:19:10] [01:19:10] ---- [run-make] run-make\issue-26092 stdout ----
[01:19:10] [01:19:10] error: make failed
[01:19:10] status: exit code: 2
[01:19:10] command: "make"
[01:19:10] stdout:
[01:19:10] ------------------------------------------
[01:19:10] PATH="/c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/issue-26092.stage2-x86_64-pc-windows-gnu:C:\projects\rust\build\x86_64-pc-windows-gnu\stage2\bin:/c/projects/rust/build/x86_64-pc-windows-gnu/stage0-tools/x86_64-pc-windows-gnu/release/deps:/c/projects/rust/build/x86_64-pc-windows-gnu/stage0-sysroot/lib/rustlib/x86_64-pc-windows-gnu/lib:/c/Program Files (x86)/Inno Setup 5:/c/Python27:/c/projects/rust/mingw64/bin:/usr/bin:/c/Perl/site/bin:/c/Perl/bin:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0:/c/Program Files/7-Zip:/c/Program Files/Microsoft/Web Platform Installer:/c/Tools/GitVersion:/c/Tools/PsTools:/c/Program Files/Git LFS:/c/Program Files (x86)/Subversion/bin:/c/Program Files/Microsoft SQL Server/120/Tools/Binn:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/110/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/Tools/Binn:/c/Program Files/Microsoft SQL Server/120/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/Tools/Binn/ManagementStudio:/c/Tools/WebDriver:/c/Program Files (x86)/Microsoft SDKs/TypeScript/1.4:/c/Program Files (x86)/Microsoft Visual Studio 12.0/Common7/IDE/PrivateAssemblies:/c/Program Files (x86)/Microsoft SDKs/Azure/CLI/wbin:/c/Ruby193/bin:/c/Tools/NUnit/bin:/c/Tools/xUnit:/c/Tools/MSpec:/c/Tools/Coverity/bin:/c/Program Files (x86)/CMake/bin:/c/go/bin:/c/Program Files/Java/jdk1.8.0/bin:/c/Python27:/c/Program Files/nodejs:/c/Program Files (x86)/iojs:/c/Program Files/iojs:/c/Users/appveyor/AppData/Roaming/npm:/c/Program Files/Microsoft SQL Server/130/Tools/Binn:/c/Program Files (x86)/MSBuild/14.0/Bin:/c/Tools/NuGet:/c/Program Files (x86)/Microsoft Visual Studio 14.0/Common7/IDE/CommonExtensions/Microsoft/TestWindow:/c/Program Files/Microsoft DNX/Dnvm:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/130/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/130/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/130/DTS/Binn:/c/Program Files/Microsoft SQL Server/130/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/110/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/DTS/Binn:/c/Program Files (x86)/Apache/Maven/bin:/c/Python27/Scripts:/c/Tools/NUnit3:/c/Program Files/Mercurial:/c/Program Files/LLVM/bin:/c/Program Files/dotnet:/c/Program Files/erl8.3/bin:/c/Tools/curl/bin:/c/Program Files/Amazon/AWSCLI:/c/Program Files/Git/cmd:/c/Program Files/Git/usr/bin:/c/Program Files (x86)/Microsoft SQL Server/140/DTS/Binn:/c/Program Files (x86)/Microsoft Visual Studio 14.0/Common7/IDE/Extensions/Microsoft/SQLDB/DAC/140:/c/ProgramData/chocolatey/bin:/c/Program Files (x86)/Yarn/bin:/c/Program Files/Microsoft Service Fabric/bin/Fabric/Fabric.Code:/c/Program Files/Microsoft SDKs/Service Fabric/Tools/ServiceFabricLocalClusterManager:/c/Program Files (x86)/nodejs:/c/Users/appveyor/AppData/Local/Yarn/bin:/c/Users/appveyor/AppData/Roaming/npm:/c/Program Files/AppVeyor/BuildAgent:/c/projects/rust:/c/projects/rust/handle" 'C:\projects\rust\build\x86_64-pc-windows-gnu\stage2\bin\rustc.exe' --out-dir /c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/issue-26092.stage2-x86_64-pc-windows-gnu -L /c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/issue-26092.stage2-x86_64-pc-windows-gnu -o "" blank.rs 2>&1 | \
[01:19:10] grep -i 'No such file or directory'
[01:19:10] [01:19:10] ------------------------------------------
[01:19:10] stderr:
[01:19:10] ------------------------------------------
[01:19:10] make: *** [Makefile:4: all] Error 1
[01:19:10] [01:19:10] ------------------------------------------
[01:19:10] [01:19:10] thread '[run-make] run-make\issue-26092' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:2435:8
[01:19:10] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:19:10] [01:19:10] [01:19:10] failures:
[01:19:10] [run-make] run-make\issue-26092
[01:19:10] [01:19:10] test result: FAILED. 158 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[01:19:10] [01:19:10] thread 'main' panicked at 'Some tests failed', src\tools\compiletest\src\main.rs:323:21

bors · 2017-09-05T15:16:17Z

⌛ Testing commit 773736c with merge f906f24...

@eddyb

Store fewer NodeIds in crate metadata. The first commit just replaces `NodeId` with `DefId` in `resolve_lifetime::Region`, so we don't run into problems when stable-hashing `TypeParameterDef` values from other crates. ~~The second commit is more interesting. It adds the `ReScopeAnon` variant to `ty::RegionKind`, which is semantically equivalent to `ReScope`. The only difference is that it does not contain a `CodeExtent` field. All `ReScope` occurrences are then replaced with `ReScopeAnon` upon metadata export. This way we don't end up with `NodeIds` from other crates in various things imported from metadata. `ReScopeAnon` can still be tested for equality.~~ This is fixed in a better way by @eddyb in #44171. r? @eddyb cc @rust-lang/compiler

bors · 2017-09-05T16:44:31Z

💔 Test failed - status-travis

alexcrichton · 2017-09-05T21:07:36Z

@bors: retry

good ol' travis troubles

bors · 2017-09-06T04:14:29Z

⌛ Testing commit 773736c with merge c132bd0de6fa65a07cb0a36ac538a99c97a12588...

bors · 2017-09-06T05:12:25Z

💔 Test failed - status-travis

michaelwoerister · 2017-09-06T10:29:29Z

Closing in favor of #44364.

@nikomatsakis

…s2, r=nikomatsakis incr.comp.: Compute fingerprint for all query results. This PR enables query result fingerprinting in incremental mode. This is an essential piece of infrastructure for red/green tracking. We don't do anything with the fingerprints yet but merging the infrastructure should protect it from bit-rotting and will make it easier to start measuring its performance impact (and thus let us determine if we should switch to a faster hashing algorithm rather sooner than later). Note, this PR also includes the changes from rust-lang#43887 which I'm therefore closing. No need to re-review the first commit though. r? @nikomatsakis

@nikomatsakis

…s2, r=nikomatsakis incr.comp.: Compute fingerprint for all query results. This PR enables query result fingerprinting in incremental mode. This is an essential piece of infrastructure for red/green tracking. We don't do anything with the fingerprints yet but merging the infrastructure should protect it from bit-rotting and will make it easier to start measuring its performance impact (and thus let us determine if we should switch to a faster hashing algorithm rather sooner than later). Note, this PR also includes the changes from rust-lang#43887 which I'm therefore closing. No need to re-review the first commit though. r? @nikomatsakis

rust-highfive assigned eddyb Aug 15, 2017

michaelwoerister force-pushed the node-id-be-gone branch 2 times, most recently from 761e9f8 to 8ecb35bCompare August 16, 2017 10:27

carols10cents added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2017

eddyb mentioned this pull request Aug 30, 2017
Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171
Merged

michaelwoerister mentioned this pull request Sep 6, 2017
incr.comp.: Compute fingerprint for all query results. #44364
Merged

michaelwoerister closed this Sep 6, 2017

Uh oh!

Uh oh!

Conversation

michaelwoerister commented Aug 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Aug 15, 2017

Uh oh!

kennytm commented Aug 15, 2017

Uh oh!

bors commented Aug 16, 2017

Uh oh!

michaelwoerister commented Aug 16, 2017

Uh oh!

eddyb commented Aug 16, 2017

Uh oh!

michaelwoerister commented Aug 16, 2017

Uh oh!

arielb1 commented Aug 16, 2017

Uh oh!

nikomatsakis commented Aug 17, 2017

Uh oh!

michaelwoerister commented Aug 17, 2017

Uh oh!

eddyb commented Aug 18, 2017

Uh oh!

alexcrichton commented Aug 24, 2017

Uh oh!

michaelwoerister commented Aug 24, 2017

Uh oh!

nikomatsakis commented Aug 24, 2017

Uh oh!

eddyb commented Aug 24, 2017

Uh oh!

carols10cents commented Aug 28, 2017

Uh oh!

eddyb commented Aug 28, 2017

Uh oh!

bors commented Aug 28, 2017

Uh oh!

nikomatsakis commented Aug 28, 2017

Uh oh!

eddyb commented Aug 29, 2017

Uh oh!

eddyb commented Aug 29, 2017

Uh oh!

eddyb commented Aug 31, 2017

Uh oh!

michaelwoerister commented Sep 4, 2017

Uh oh!

bors commented Sep 4, 2017

Uh oh!

bors commented Sep 4, 2017

Uh oh!

kennytm commented Sep 4, 2017

Uh oh!

bors commented Sep 4, 2017

Uh oh!

bors commented Sep 4, 2017

Uh oh!

kennytm commented Sep 4, 2017

Uh oh!

bors commented Sep 5, 2017

Uh oh!

bors commented Sep 5, 2017

Uh oh!

kennytm commented Sep 5, 2017

Uh oh!

bors commented Sep 5, 2017

Uh oh!

bors commented Sep 5, 2017

Uh oh!

alexcrichton commented Sep 5, 2017

Uh oh!

bors commented Sep 6, 2017

Uh oh!

bors commented Sep 6, 2017

Uh oh!

michaelwoerister commented Sep 6, 2017

Uh oh!

michaelwoerister commented Aug 15, 2017•
edited
Loading