On demandify reachability by cramertj · Pull Request #40873 · rust-lang/rust

cramertj · 2017-03-28T07:00:03Z

I tried following this guidance from #40746:

The following tasks currently execute before a tcx is built, but they could be easily converted into queries that are requested after tcx is built. The main reason they are the way they are was to avoid a gratuitious refcell (but using the refcell map seems fine)...

but the result of moving region_maps out of TyCtxt and into a query caused a lot of churn, and seems like it could potentially result in a rather large performance hit, since it means a dep-graph lookup on every use of region_maps (rather than just a field access). Possibly TyCtxt could store a RefCell<Option<RegionMap>> internally and use that to prevent repeat lookups, but that feels like it's duplicating the work of the dep-graph. @nikomatsakis What did you have in mind for this?

rust-highfive · 2017-03-28T07:00:08Z

r? @eddyb

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

cramertj · 2017-03-28T07:00:18Z

r? @nikomatsakis

nikomatsakis · 2017-03-28T10:02:20Z

Regarding the region-maps:

First, we should measure the performance. It may or may not be significant.
Second, it seems like it is becoming quite common to have things that are effectively "lazilly computed globals". Right now we are handling those uniformly (with the cache key being a crate number) but maybe it's worth treating them as an (encapsulated) Option field.
Third, I expect that some amount of local caching (i.e., if we access region_maps() many times in a loop, we can store it into a local variable) would solve the problem.

Certainly we should solve this problem, because I think this is roughly the pattern we want going forward. But I think it won't be hard to solve.

eddyb · 2017-03-28T14:32:47Z

IMO we would have all the maps per-crate, and HashMap<(), V> should be quite efficient - even if it's not, we could use specialization.

nikomatsakis · 2017-03-28T22:15:59Z

@eddyb seems reasonable, yes

cramertj · 2017-03-29T00:15:07Z

@eddyb Should I take that to mean that I should leave it as-is for now (module fixing the tidy check)? Also, is there interest in having queries include a get_ref function or similar to access the result, rather than having to wrap in an Rc?

bors · 2017-03-29T00:17:48Z

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

nikomatsakis · 2017-03-29T13:24:51Z

@cramertjRc is better, I think, because it avoids refcell issues, and it's simpler to have all of the queries pass ownership.

bors · 2017-03-29T14:32:31Z

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

cramertj · 2017-04-04T14:41:40Z

Removed region-mapping in favor of #41057

nikomatsakis · 2017-04-06T13:22:28Z

@bors r+

bors · 2017-04-06T13:22:29Z

📌 Commit aab2cca has been approved by nikomatsakis

@nikomatsakis

…komatsakis On demandify reachability cc rust-lang#40746 I tried following this guidance from rust-lang#40746: > The following tasks currently execute before a tcx is built, but they could be easily converted into queries that are requested after tcx is built. The main reason they are the way they are was to avoid a gratuitious refcell (but using the refcell map seems fine)... but the result of moving `region_maps` out of `TyCtxt` and into a query caused a lot of churn, and seems like it could potentially result in a rather large performance hit, since it means a dep-graph lookup on every use of `region_maps` (rather than just a field access). Possibly `TyCtxt` could store a `RefCell<Option<RegionMap>>` internally and use that to prevent repeat lookups, but that feels like it's duplicating the work of the dep-graph. @nikomatsakis What did you have in mind for this?

bors · 2017-04-06T19:18:24Z

⌛ Testing commit aab2cca with merge 408c882...

bors · 2017-04-06T19:37:21Z

💔 Test failed - status-appveyor

alexcrichton · 2017-04-07T02:48:56Z

@bors: retry * ar.exe error #40546

…

On Thu, Apr 6, 2017 at 1:37 PM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2804> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#40873 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95ECs5QAye9DVa2FVWNENcAD47mVUks5rtT7zgaJpZM4MrNru> .

bors · 2017-04-07T08:36:16Z

⌛ Testing commit aab2cca with merge 4c59c92...

@nikomatsakis

On demandify reachability cc #40746 I tried following this guidance from #40746: > The following tasks currently execute before a tcx is built, but they could be easily converted into queries that are requested after tcx is built. The main reason they are the way they are was to avoid a gratuitious refcell (but using the refcell map seems fine)... but the result of moving `region_maps` out of `TyCtxt` and into a query caused a lot of churn, and seems like it could potentially result in a rather large performance hit, since it means a dep-graph lookup on every use of `region_maps` (rather than just a field access). Possibly `TyCtxt` could store a `RefCell<Option<RegionMap>>` internally and use that to prevent repeat lookups, but that feels like it's duplicating the work of the dep-graph. @nikomatsakis What did you have in mind for this?

bors · 2017-04-07T12:51:15Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4c59c92 to master...

aidanhs · 2017-04-07T12:55:28Z

This (spuriously) hung halfway through one of the OSX tests (guess which one), possibly related to #40571, cc @alexcrichton. It then recovered after about 45mins and continued.

alexcrichton · 2017-04-08T20:45:43Z

@aidanhs the full logs for that build are super weird. I wonder if there's a few travis bugs in play here. I'm seeing no discontinuities in our logging (which all have timestamps), but I'm seeing stuff like:

[00:56:40] Finished release [optimized] target(s) in 0.1 secs
[00:32:05] Compiling rustc_llvm v0.0.0 (file:///Users/travis/build/rust-lang/rust/src/librustc_llvm)
[00:56:40] Copying stage0 std from stage0 (i686-apple-darwin -> i686-apple-darwin / i686-apple-darwin)
[00:32:11] Compiling syntax_pos v0.0.0 (file:///Users/travis/build/rust-lang/rust/src/libsyntax_pos)
[00:56:41] Building stage0 test artifacts (i686-apple-darwin -> i686-apple-darwin)
[00:56:41] Finished release [optimized] target(s) in 0.0 secs
[00:32:17] Compiling rustc_errors v0.0.0 (file:///Users/travis/build/rust-lang/rust/src/librustc_errors)

No idea why it's jumping back and forth...

rust-highfive assigned eddyb Mar 28, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Mar 28, 2017

cramertj force-pushed the on-demandify-queries branch from 402a57a to 8f4b72eCompare March 29, 2017 05:57

This was referenced Apr 4, 2017
migrate region-maps to on-demand #41057
Closed
migrate all "incremental tasks" to "on-demand queries" #40746
Closed

cramertj force-pushed the on-demandify-queries branch 2 times, most recently from ed2f96b to afebb2bCompare April 4, 2017 14:40

cramertj changed the title ~~On demandify reachability and [WIP] region mapping~~On demandify reachabilityApr 4, 2017

nikomatsakis closed this Apr 4, 2017

nikomatsakis reopened this Apr 4, 2017

On-demandify reachability
aab2cca

cramertj force-pushed the on-demandify-queries branch from afebb2b to aab2ccaCompare April 4, 2017 14:46

nikomatsakis approved these changes Apr 6, 2017
View reviewed changes

frewsxcv mentioned this pull request Apr 6, 2017
Rollup of 9 pull requests #41115
Closed

carols10cents mentioned this pull request Apr 7, 2017
Spurious failures on AppVeyor due to ar.exe errors #40546
Closed

bors merged commit aab2cca into rust-lang:masterApr 7, 2017

aidanhs mentioned this pull request Apr 20, 2017
OSX builds occasionally hang #41422
Closed

cramertj deleted the on-demandify-queries branch September 21, 2017 16:10

Uh oh!

Uh oh!

Conversation

cramertj commented Mar 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 28, 2017

Uh oh!

cramertj commented Mar 28, 2017

Uh oh!

nikomatsakis commented Mar 28, 2017

Uh oh!

eddyb commented Mar 28, 2017

Uh oh!

nikomatsakis commented Mar 28, 2017

Uh oh!

cramertj commented Mar 29, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Mar 29, 2017

Uh oh!

nikomatsakis commented Mar 29, 2017

Uh oh!

bors commented Mar 29, 2017

Uh oh!

cramertj commented Apr 4, 2017

Uh oh!

nikomatsakis commented Apr 6, 2017

Uh oh!

bors commented Apr 6, 2017

Uh oh!

bors commented Apr 6, 2017

Uh oh!

bors commented Apr 6, 2017

Uh oh!

alexcrichton commented Apr 7, 2017 via email

Uh oh!

bors commented Apr 7, 2017

Uh oh!

bors commented Apr 7, 2017

Uh oh!

aidanhs commented Apr 7, 2017

Uh oh!

alexcrichton commented Apr 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cramertj commented Mar 28, 2017•
edited
Loading

cramertj commented Mar 29, 2017•
edited
Loading