Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

some basic dag optimizations #23811

Merged
merged 7 commits into from Feb 19, 2020
Merged

some basic dag optimizations #23811

merged 7 commits into from Feb 19, 2020

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jan 8, 2020

This removes a massive amount of unnecessary allocations from the graph operations. The graph is based on set data structures, and every time a set was iterated over (which is needed for most any graph operation) a new slice was allocated, and the set elements were copied into the slice.

The fundamental change here is making the dag.Set type a simple map, so that it can be iterated over directly, and passed between functions without first converting it to a slice. This also allows us to continue to use the same basic graph data structures, without overhauling everything to a new interface that would be required for a more efficient or safer dag design.

A rough benchmark from the CLI using a config with 400 dependent instances that takes 2m54s to build the apply graph before this change only takes 11s after the change with 1/2 the memory allocation (and much less pressure on GC).

@jbardin jbardin added this to the v0.13.0 milestone Jan 8, 2020
@jbardin jbardin requested a review from a team January 8, 2020 16:03
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Jan 8, 2020
@jbardin
Copy link
Member Author

jbardin commented Jan 8, 2020

Added a fairly coarse benchmark, comprised of the 2 most expensive operations usually done by core -- find all node dependencies, and reduce the graph. While not perfect, it provides some numbers to start with.

benchmark          old ns/op        new ns/op       delta
BenchmarkDAG-4     149419826910     18200804310     -87.82%

benchmark          old allocs     new allocs     delta
BenchmarkDAG-4     1246639178     102830948      -91.75%

benchmark          old bytes       new bytes      delta
BenchmarkDAG-4     13892716968     6188975040     -55.45%

s := new(Set)
start := AsVertexList(g.DownEdges(v))
func (g *AcyclicGraph) Ancestors(v Vertex) (Set, error) {
s := make(Set)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go question -- make is for "type slice, map, or chan (only)" https://golang.org/pkg/builtin/#make. Why is this okay to use the dag package's Set type here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying type of Set is map[interface{}]interface{}, so it is equivalent to calling Set(make(map[interface{}]interface{})). The same can be done with named slice and chan types (though the latter would be pretty unusual).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -335,6 +335,63 @@ func TestAcyclicGraphWalk_error(t *testing.T) {

}

func BenchmarkDAG(b *testing.B) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay benchmark test!

dag/walk.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

This allows iteration directly over the set, removing the need to
allocate and copy a new slice for every iteration.
Since the set can be iterated over directly, we no longer need to copy
the values into a new slice.
Make the default walks always use the sets directly, which avoids
repeated copying of every set into a new slice.
Remove the abandoned graph debugger
Also removing unnecessary uses of the Set.List
There's no need for the json2dot command since we can't create json
debug graphs.
@jbardin jbardin merged commit 46212a6 into master Feb 19, 2020
@jbardin jbardin deleted the jbardin/dag branch February 19, 2020 20:20
@ghost
Copy link

ghost commented Apr 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@hashicorp hashicorp locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants