first pass at isolating dep-graph tasks by nikomatsakis · Pull Request #40308 · rust-lang/rust

nikomatsakis · 2017-03-06T20:59:02Z

This intentionally leaves DepGraph::in_task(), the more common form,
alone. Eventually all uses of DepGraph::in_task() should be ported
to with_task(), but I wanted to start with a smaller subset.

I also used AssertDepGraphSafe on the closures that are found in
trans. This is because the types there are non-trivial and I wanted to
lay down the mechanism and come back to the more subtle cases.

The current approach taken in this PR has a downside: it is necessary
to manually "reify" fn types into fn pointers when starting a task,
like so:

dep_graph.with_task(..., task_fn as fn(_))

this is because with_task takes some type T that implements
DepGraphTask rather than taking a fn() type directly. This is so
that we can accept closure and also so that we can accept fns with
multiple arities. I am not sure this is the right approach.

Originally I wanted to use closures bound by an auto trait, but that
approach has some limitations:

the trait cannot have a read() method; since the current method
is unused, that may not be a problem.
more importantly, we would want the auto trait to be "undefined" for all types
by default -- that is, this use case doesn't really fit the typical
auto trait scenario. For example, imagine that there is a u32 loaded
out of a hir::Node -- we don't really want to be passing that
u32 into the task!

nikomatsakis · 2017-03-06T20:59:14Z

r? @michaelwoerister

cc @eddyb

cc #40304

rust-highfive · 2017-03-06T20:59:14Z

r? @nrc

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

nikomatsakis · 2017-03-06T21:01:16Z

This is probably not ready to merge quite yet, but I wanted to let people take a look and get some feedback. I'm thinking of experimenting with different forms for more ergonomic APIs. I had thoughts of experimenting with macros that create functions, though it might be better to take advatnage of the new "coerce closure into fn ptr" RFC, once that is implemented. Another possibility is always passing the tcx, which is basically always necessary.

michaelwoerister · 2017-03-07T13:32:33Z

Another possibility is always passing the tcx, which is basically always necessary.

If that's possible it would make for a rather simple API.

eddyb · 2017-03-07T13:44:36Z

@nikomatsakis That RFC is already implemented! We'll be able to use it once we bump the beta.

nikomatsakis · 2017-03-07T14:38:01Z

OK, pushed a revised version that takes a fn with two arguments. This might get mildly cleaner if we had access to the closure-as-fn thing (good to hear that will come soon, @eddyb) but, actually, I think not that much. Declaring a fn item just isn't that bad, and for those cases where it is, it starts to suggest places that we should be employing on-demand, I think.

nikomatsakis · 2017-03-07T21:08:33Z

removing "WIP" as I think this is ready for review

r? @michaelwoerister

michaelwoerister

Seems like a good starting point. Closure-to-fn coercions will certainly make this more ergonomic.

r=me with the comments addressed.

michaelwoerister · 2017-03-08T08:01:08Z

I don't see any macros in that module.

michaelwoerister · 2017-03-08T08:02:58Z

A comment explaining the setup would be good here, especially why there's a distinction between cx and arg.

michaelwoerister · 2017-03-08T08:03:37Z

It's not an auto trait anymore, right?

good point :)

michaelwoerister · 2017-03-08T08:05:29Z

Not needed anymore?

ah yeah all the macro stuff is from an older pass, thanks

michaelwoerister · 2017-03-08T08:21:53Z

This explicit read is not there anymore in the new version. This should be fine -- after all, the new system should guarantee that we cannot do anything with the NodeId without triggering a read -- but I wanted to double check that that is intentional.

that is intentional -- the newer code no longer "leaks" in a hir::Item (or TraitItem, etc), so there is no need for the "automatic" read.

nikomatsakis · 2017-03-08T14:16:08Z

@bors r=mw

bors · 2017-03-08T14:16:09Z

📌 Commit 19c6bfb has been approved by mw

bors · 2017-03-09T10:42:32Z

🔒 Merge conflict

alexcrichton · 2017-03-09T15:04:23Z

@bors: retry

…, r=mw first pass at isolating dep-graph tasks This intentionally leaves `DepGraph::in_task()`, the more common form, alone. Eventually all uses of `DepGraph::in_task()` should be ported to `with_task()`, but I wanted to start with a smaller subset. I also used `AssertDepGraphSafe` on the closures that are found in trans. This is because the types there are non-trivial and I wanted to lay down the mechanism and come back to the more subtle cases. The current approach taken in this PR has a downside: it is necessary to manually "reify" fn types into fn pointers when starting a task, like so: dep_graph.with_task(..., task_fn as fn(_)) this is because `with_task` takes some type `T` that implements `DepGraphTask` rather than taking a `fn()` type directly. *This* is so that we can accept closure and also so that we can accept fns with multiple arities. I am not sure this is the right approach. Originally I wanted to use closures bound by an auto trait, but that approach has some limitations: - the trait cannot have a `read()` method; since the current method is unused, that may not be a problem. - more importantly, we would want the auto trait to be "undefined" for all types *by default* -- that is, this use case doesn't really fit the typical auto trait scenario. For example, imagine that there is a `u32` loaded out of a `hir::Node` -- we don't really want to be passing that `u32` into the task!

A task function is now given as a `fn` pointer to ensure that it carries no state. Each fn can take two arguments, because that worked out to be convenient -- these two arguments must be of some type that is `DepGraphSafe`, a new trait that is intended to prevent "leaking" information into the task that was derived from tracked state. This intentionally leaves `DepGraph::in_task()`, the more common form, alone. Eventually all uses of `DepGraph::in_task()` should be ported to `with_task()`, but I wanted to start with a smaller subset. Originally I wanted to use closures bound by an auto trait, but that approach has some limitations: - the trait cannot have a `read()` method; since the current method is unused, that may not be a problem. - more importantly, we would want the auto trait to be "undefined" for all types *by default* -- that is, this use case doesn't really fit the typical auto trait scenario. For example, imagine that there is a `u32` loaded out of a `hir::Node` -- we don't really want to be passing that `u32` into the task!

alexcrichton · 2017-03-10T16:15:36Z

@bors: r=mw

bors · 2017-03-10T16:15:37Z

📌 Commit 4d5441f has been approved by mw

…, r=mw first pass at isolating dep-graph tasks This intentionally leaves `DepGraph::in_task()`, the more common form, alone. Eventually all uses of `DepGraph::in_task()` should be ported to `with_task()`, but I wanted to start with a smaller subset. I also used `AssertDepGraphSafe` on the closures that are found in trans. This is because the types there are non-trivial and I wanted to lay down the mechanism and come back to the more subtle cases. The current approach taken in this PR has a downside: it is necessary to manually "reify" fn types into fn pointers when starting a task, like so: dep_graph.with_task(..., task_fn as fn(_)) this is because `with_task` takes some type `T` that implements `DepGraphTask` rather than taking a `fn()` type directly. *This* is so that we can accept closure and also so that we can accept fns with multiple arities. I am not sure this is the right approach. Originally I wanted to use closures bound by an auto trait, but that approach has some limitations: - the trait cannot have a `read()` method; since the current method is unused, that may not be a problem. - more importantly, we would want the auto trait to be "undefined" for all types *by default* -- that is, this use case doesn't really fit the typical auto trait scenario. For example, imagine that there is a `u32` loaded out of a `hir::Node` -- we don't really want to be passing that `u32` into the task!

bors · 2017-03-11T15:50:37Z

⌛ Testing commit 4d5441f with merge 4b1dfbd...

first pass at isolating dep-graph tasks This intentionally leaves `DepGraph::in_task()`, the more common form, alone. Eventually all uses of `DepGraph::in_task()` should be ported to `with_task()`, but I wanted to start with a smaller subset. I also used `AssertDepGraphSafe` on the closures that are found in trans. This is because the types there are non-trivial and I wanted to lay down the mechanism and come back to the more subtle cases. The current approach taken in this PR has a downside: it is necessary to manually "reify" fn types into fn pointers when starting a task, like so: dep_graph.with_task(..., task_fn as fn(_)) this is because `with_task` takes some type `T` that implements `DepGraphTask` rather than taking a `fn()` type directly. *This* is so that we can accept closure and also so that we can accept fns with multiple arities. I am not sure this is the right approach. Originally I wanted to use closures bound by an auto trait, but that approach has some limitations: - the trait cannot have a `read()` method; since the current method is unused, that may not be a problem. - more importantly, we would want the auto trait to be "undefined" for all types *by default* -- that is, this use case doesn't really fit the typical auto trait scenario. For example, imagine that there is a `u32` loaded out of a `hir::Node` -- we don't really want to be passing that `u32` into the task!

bors · 2017-03-11T18:10:01Z

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

rust-highfive assigned nrc Mar 6, 2017

rust-highfive assigned michaelwoerister and unassigned nrc Mar 6, 2017

nikomatsakis changed the title ~~WIP: first pass at isolating dep-graph tasks~~first pass at isolating dep-graph tasksMar 7, 2017

michaelwoerister approved these changes Mar 8, 2017
View reviewed changes

nikomatsakis force-pushed the incr-comp-isolate-task branch from 635a4c1 to 19c6bfbCompare March 8, 2017 14:15

alexcrichton mentioned this pull request Mar 10, 2017
Rollup of 36 pull requests #40418
Closed

nikomatsakis added 2 commits March 10, 2017 08:15

add comments and remove unused code paths
4d5441f

alexcrichton force-pushed the incr-comp-isolate-task branch from 19c6bfb to 4d5441fCompare March 10, 2017 16:15

alexcrichton mentioned this pull request Mar 10, 2017
Rollup of 35 pull requests #40420
Closed

bors merged commit 4d5441f into rust-lang:masterMar 11, 2017

bors mentioned this pull request Mar 11, 2017
Convert Dependency Graph Tasks from in_tasks to with_tasks #40376
Closed

nikomatsakis deleted the incr-comp-isolate-task branch April 14, 2017 10:12

Uh oh!

Uh oh!

Conversation

nikomatsakis commented Mar 6, 2017

Uh oh!

nikomatsakis commented Mar 6, 2017

Uh oh!

rust-highfive commented Mar 6, 2017

Uh oh!

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

Uh oh!

Uh oh!

michaelwoerister commented Mar 7, 2017

Uh oh!

eddyb commented Mar 7, 2017

Uh oh!

nikomatsakis commented Mar 7, 2017

Uh oh!

nikomatsakis commented Mar 7, 2017

Uh oh!

michaelwoerister left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Mar 8, 2017

Uh oh!

bors commented Mar 8, 2017

Uh oh!

bors commented Mar 9, 2017

Uh oh!

alexcrichton commented Mar 9, 2017

Uh oh!

alexcrichton commented Mar 10, 2017

Uh oh!

bors commented Mar 10, 2017

Uh oh!

bors commented Mar 11, 2017

Uh oh!

bors commented Mar 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nikomatsakis commented Mar 6, 2017•
edited
Loading