last few MergedGraph() implementations #703
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Representing an MCL function value as a golang function from Value to
Value was a mistake, it should be a function from Vertex to Vertex.
Here is why this is a mistake:
The output of a function like
$f = fn(x) {
Shell(Sprintf("seq %d", x))
}
varies over time, while a single Value does not. Thus, code like
Map($f, list(1, 2))
would first produce the value list("1", "1"), but then it would
_not_ update to list("1", "2") when "seq 2" produces its second
line. That's because with the mistaken design, when Map receives a
new FuncValue or a new input list of N elements, Map calls the
function from Value to Value N times and produces a single output
list of N elements.
Here is why the corrected design is better:
Here's what happens with this new design when Map receives a new
FuncValue or a new input list of N elements.
First, Map constructs N item-input nodes, each of which extracts a
different entry from the list. Then, Map calls the function from
Vertex to Vertex N times, once for each item-input node, and thus
obtain N item-output nodes. Finally, Map constructs an
item-collecting node which constructs a list out of all of its
inputs, and Map connects the N item-output nodes to the
item-collecting node. This item-collecting node is the output of
Map.
The Vertex to Vertex function constructs and connects its own nodes;
in this case, it constructs an Sprintf node and connects the
item-input node to it, and then constructs a Shell node and connects
the Sprintf node to it, and then returns the Shell node as the
item-output node.
The two Shell node in this sub-graph emit a first value "1", which
propagates to the item-collecting node and causes it to output a
first value list("1", "1"). Then, the second Shell node emits a
second value "2", which propagates to the item-collecting node and
causes it to output a second value list("1", "2"), as desired.
Here is how this commit brings us closer to the above plan:
Changing FuncValue throughout the codebase is a big change. One of
the difficulties is that it is not just nodes which are emitting
FuncValues, there are also many other places in the code where
FuncValue is used to hold a golang function from Value to Value.
Some of those places now need to hold a golang function from Vertex
to Vertex, but other places still need to hold a golang function
from Vertex to Vertex.
Thus, as a first step, we need to split FuncValue into two types.
This commit splits the old FuncValue into two types:
1. The new FuncValue will hold a function from Vertex to Vertex.
FuncValue is a Value.
2. A new type named "SimpleFn" will hold a function from Value to
Value. SimpleFn is not a Value.
This commit replaces occurrences of the old FuncValue with one of
those two new types, as appropriate. This commit does not yet adapt
the surrounding code to make use of the new representation; that
will be done in future commits. I have annotated the missing parts
with the following panic message in order to make it easy to find
which parts still need to be implemented. The "..." part explains
what needs to be implemented.
panic("TODO [SimpleFn]: ...");
Here's where I need help:
One part of the code which is not clear to me are the parts which
use reflection. I don't understand the purpose of that code well
enough to explain what needs to be implemented. I have annotated
those "known unknown" parts of the remaining work with the following
panic message in order to make it easy to find which parts still
need more thinking and planning:
panic("TODO [SimpleFn] [Reflect]: ...");
This will eventually let functions change the running graph via a transaction API. At the moment the core Lock and Unlock primitives aren't implemented.
This is useful for the common case in which we call one FuncValue to construct a bunch of nodes, and later we switch to a different FuncValue and so we want to remove all the nodes added by the first FuncValue and replace them by the nodes added by the second FuncValue.
Merging those two packages allows us to avoid import cycles when a Func needs to add an Expr to the graph.
FuncValues are now manipulating the graph instead of manipulating values, so the logic for calling a FuncValue must now follow suit.
it's the exact same thing as ExprCall.Args
This adds a MergedGraph signature to the Expr interface. The txn type is
interface{} until we have that merged.
This adds a MergedGraph signature to the Stmt interface. The txn type is
interface{} until we have that merged.
This puts it into play, but doesn't initialize the input args at all.
This reverts commit 3ea3845.
This reverts commit a62889e.
|
Sweet! I'm making good progress on the new engine. The race detector helped me find some races today and I think I got those fixed. One last API addition to the new engine library, and then I can port it in and maybe we can put all of this together! |
|
This PR has now been incorporated into #704. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Building on top of #701.
This implements MergedGraph() for ExprCall, ExprFunc, and ExprVar.