Improve RuleGraph convergence time slightly #10700
Conversation
stuhood
force-pushed
the
stuhood/rule-graph-converge
branch
from
34ba70b
to
19aec1c
Compare
…prune dependencies earlier. [ci skip-build-wheels]
[ci skip-build-wheels]
[ci skip-build-wheels]
stuhood
force-pushed
the
stuhood/rule-graph-converge
branch
from
19aec1c
to
592db7b
Compare
|
|
||
| // Should be called after a Node has been successfully reduced (regardless of whether it became | ||
| // monomorphic) to maybe mark it minimal. | ||
| let maybe_mark_minimal_in_set = |minimal_in_set: &mut HashSet<NodeIndex<u32>>, |
There was a problem hiding this comment.
I don't think this closure is actually closing over any variables here, so why not define it as a function scoped within the outer monomorphize function?
| @@ -416,26 +447,43 @@ impl<'t, R: Rule> Builder<'t, R> { | |||
| } else { | |||
| continue; | |||
| }; | |||
| if !matches!(node.node, Node::Rule(_)) { | |||
| continue; | |||
| match node.node { | |||
There was a problem hiding this comment.
Nit: it would be nice to put the fall-through case at the bottom of the match. I also prefer to have an empty match statement return a () rather than an empty {} block, since the former is less verbose.
| @@ -33,7 +33,7 @@ If a user makes a request to the engine that does not have a corresponding `Quer | |||
|
|
|||
| `Params` are typed, comparable (`eq`/`hash`) values that represent both the inputs to `Rules`, and the building block of the runtime memoization key for a `Rule`. The set of `Params` (unique by type) that are consumed to create a `Rule`'s inputs (plus the `Rule`'s own identity) make up the memoization key for a runtime instance of the `Rule`. | |||
|
|
|||
| It's important to note though that the `Params` in a `Rule` instance's identity will _not_ always become the positional arguments to that `Rule`: in many cases, a `Param` will be consumed by a `Rule`'s dependencies in order to produce an output value that becomes either a positional argument to the `Rule` as it starts, or the result of a `Get` while a coroutine `Rule` runs. | |||
There was a problem hiding this comment.
This paragraph had confused me. Good rewrite.
| @@ -83,6 +83,7 @@ async def fibonacci(n: int) -> Fibonacci: | |||
| x, y = await Get(Fib, int(n - 2)), await Get(Fib, int(n - 1)) | |||
| return Fibonacci(x.val + y.val) | |||
| ``` | |||
|
|
|||
| ``` | |||
There was a problem hiding this comment.
Add python to this, and Readme.io will make it look much prettier. Ditto on the above.
|
|
||
| #### Monotonicity vs Global Information | ||
| One minimal problematic testcase (`fn wide_cycle` in `src/rust/engine/rule_graph/src/tests.rs`) looks like: |
There was a problem hiding this comment.
Maybe add an example of a non-problematic test case. For example, show what the graph looks like for the Fibonannaci example.
Also, consider adding pictures of the visualized graph. Readme.io allows you to upload pictures.
|
Going to land this to continue building atop it. Thanks for the feedback: will apply in followups. |
Problem
One of the open issues in https://www.pantsbuild.org/v2.0/docs/internal-rules-architecture was that we weren't able to prune based on transitive information.
Solution
Track whether the
in_setof a node is transitively minimal, which allows us to prune nodes that don't consume a providedParamsooner. Additionally, document some of the other things I've explored recently, and add a minimal testcase that goes into an infinite loop without thesplitsspecial case.Result
This does not fix #10683 unfortunately. Still on it.