pass1 and pass2 no longer cause a stack overflow for large graphs #228

rustedgrail · 2018-01-24T19:50:44Z

This PR causes pass1 and pass2 to return functions instead of making recursive calls. By returning functions, there is a fixed number of function calls on the stack, regardless of the size of the graph. This is a fix for issue 223.

I ran the tests, and they all pass. Thank you for having comprehensive tests!

@miriammelnick paired with me on the changes.

…e scope was being created

miriammelnick

Code LGTM. Left a few suggestions for comments.

miriammelnick · 2018-01-24T20:17:12Z

lib/position/bk.js

+  // These returned functions are the recursive and base function calls. By
+  // returning the functions and calling them here, we do not add additional
+  // function calls to the stack. If recursive case generates additional
+  // functions calls, they will be prepended to the array.


Nitpicking the last sentence of this comment:
If the recursive case generates additional functions calls, they will be prepended to the array.

miriammelnick · 2018-01-24T20:30:01Z

lib/position/bk.js

-      var min = _.reduce(blockG.outEdges(v), function(min, e) {
-        pass2(e.w);
-        return Math.min(min, xs[e.w] - blockG.edge(e));
+    return _.map(blockG.outEdges(v).reverse(), function(e) {


Why reverse?

This was left over from a previous commit. I removed the reverse.

miriammelnick · 2018-01-24T20:30:30Z

lib/position/bk.js

+    return _.map(blockG.outEdges(v).reverse(), function(e) {
+      return function() { return pass2(e.w); };
+    }).concat([function () {
+      var min =_.reduce(blockG.outEdges(v), function(acc, e) {


Add a space between = and _

miriammelnick · 2018-01-24T20:33:25Z

lib/position/bk.js

+
+  // This function handles iterating over the graph without adding functions
+  // calls to the stack. The argument, helper, returns an array of functions.
+  // These returned functions are the recursive and base function calls. By


Consider renaming "helper" to something more descriptive... maybe something like getNextStepTraversalFuncs . Thoughts?

miriammelnick · 2018-01-24T20:39:46Z

lib/position/bk.js

+      var traversalFuncs = helper(v);
+
+      while(traversalFuncs.length) {
+        var funcsOrX = traversalFuncs.shift()();


funcsOrX is a non-homogenous array, so I'd recommend adding a comment explicitly stating what types of things it can contain.

miriammelnick · 2018-01-24T20:40:24Z

lib/position/bk.js

+      }, 0);
+      return xs[v];
+    }]);
+  }


I tried adding whitespace in various places but it didn't seem to make it clearer. Probably just leave it as-is.

miriammelnick · 2018-01-24T20:40:46Z

lib/position/bk.js

+  // function that will set xs[v] with the smallest coordinates.
+  function pass1(v) {
+    return _.map(blockG.inEdges(v).reverse(), function(e) {
+      return function() { return pass1(e.v);};


nit: add space after semicolon before close-curly-brace.

miriammelnick · 2018-01-24T20:43:28Z

lib/position/bk.js

+  }
+
+  // pass2 maps over the outEdges of a node and creates a recursive function
+  // that will be called by iterate. At the end of those functions, there is


Phrasing is awkward.
~~At the end of those functions, there is another function that will set~~ Finally, it sets xs[v] with the greatest coordinates.

Also, thoughts on "to the greatest coordinates" versus "with the greatest coordinates"? Both here and above, in the comment for pass1.

lutzroeder · 2018-01-25T08:05:50Z

I tried this change in Netron and seems to freeze for large graphs. Any ideas why this happens?

Repro steps:

curl https://storage.googleapis.com/download.tensorflow.org/models/inception_v3_2016_08_28_frozen.pb.tar.gz | tar xvz
git clone git@github.com:lutzroeder/Netron.git && cd Netron && npm install
npx electron .
Open file downloaded in step 1 --> renders in ~5 seconds
Close the app
Overwrite ./node_modules/dagre/dist with the updated bits
npx electron .
Open the file downloaded in step 1 --> render does not return in more than a minute

miriammelnick · 2018-01-25T17:12:34Z

@lutzroeder
How does performance with different sized graphs compare using the old function (before this PR) and the new one?

lutzroeder · 2018-01-25T21:26:58Z

For the example above time seems to have gone from ~2.5 seconds (old) to infinite (new). Haven't had time to dig in if that's a performance issue or a deadlock and what happens with different files but it seems to be a regression you should investigate.

rustedgrail · 2018-01-30T02:08:36Z

I can reproduce the issue, but I'm not sure how to debug Netron. My guess is that in the original, visited prevented an infinite of loop. In this PR though, visited is only checked in a parent scope from the recursion and it's not exiting the loop for nodes it's already seen.

lutzroeder · 2018-01-30T03:20:36Z

Netron is an Electron app so vscode with the Debugger for Chrome extension is one option (run npm install first and set the debugger to Debug Renderer Process). Alternatively you can try to install the python server via pip install netron, run netron inception_v3_2016_08_28_frozen.pb and debug in the browser. Breakpoint on dagre.layout(graph) in view-render.js which is calling into this code.

rustedgrail · 2018-01-31T17:40:44Z

I saw a lot of repeated 'v' in pass1 when I debugged. Once I moved the visited check to the pass functions, the graph was able to render quickly. Does this commit work you you @lutzroeder?

cpettitt

Going to an iterative approach is a nice improvement. However, if I'm not mistaken this could be done more compactly and efficiently by putting the nodes on a stack vs. concatenating lists of traversal functions. Also, I would expect treating the list as a stack vs a queue to be more efficient (Array pop vs shift). A stack would also make this DFS which is consistent with the previous approach. I don't recall if DFS vs BFS was important for these passes.

rustedgrail · 2018-02-02T19:44:44Z

The way the traversal functions are being added is LIFO. The new functions are added to the front of the array and then we shift off the front of the array.

We could build an ordered array of node ids, appending to it all children node ids. Then reverse the list and iterate over it to calculate the value for xs starting with the lowest children nodes. Is that what you meant? That changes the behavior from setting each xs value as soon as we can to setting all xs values at the end.

cpettitt · 2018-02-03T16:09:07Z

This is roughly what I was imagining for pass 1. It keeps all of the code for the pass in one place, avoids creating closures, and avoids the more expensive shift call. I have not tested this, so it would need to be verified.

// Pass 1
var xs = {};
var visited = {};
var stack = blockG.nodes();
var elem;
while ((elem = stack.pop())) {
    if (xs.hasOwnProperty(elem)) {
        continue;
    }
    if (visited[elem]) {
        xs[elem] = blockG.inEdges(elem).reduce((max, e) => {
            return Math.max(max, xs[e.v] + blockG.edge(e));
        }, 0);
    } else {
        visited[elem] = true;
        stack.push(elem);
        stack = stack.concat(blockG.predecessors(elem));
    }
}

BTW, this is a bit more complicated than I originally imagined. I didn't look closely enough at the algorithm when I reviewed last time.

rustedgrail · 2018-02-05T22:01:25Z

I updated the PR with very nearly what you said. I separated pass1 and pass2 because having a reduce inside the while loop causes a warning. Reduce take a function and it references xs and blockG, which are outer scoped variables. Let me know if there is anything else I should update.

"Functions declared within loops referencing an outer scoped variable may lead to confusing semantics."

cpettitt · 2018-02-06T16:21:09Z

You're right, that function should not have been in a while loop. Thanks for the enhancement. I'll schedule this for release shortly.

rustedgrail added 8 commits January 23, 2018 17:40

1 failing test (probably an issue setting xs in pass 1)

755a0e9

Moved the min and max to be tracked on the same level as new recursiv…

6bdaebf

…e scope was being created

Fixing the tests

0bec66b

Merged the iterate functions

e901209

using map instead of reduce

c0edae2

Adding comments

627b1de

Reverting the package-lock

2112ae6

Cleaning up comments

d7deee0

miriammelnick suggested changes Jan 24, 2018

View reviewed changes

Updating comments and spacing based on feedback

501f962

lutzroeder assigned lutzroeder and cpettitt and unassigned lutzroeder Jan 25, 2018

lutzroeder unassigned cpettitt Jan 25, 2018

Moving visited to the pass functions instead of the iterate function

064d499

lutzroeder requested a review from cpettitt February 1, 2018 03:09

lutzroeder assigned cpettitt Feb 1, 2018

cpettitt requested changes Feb 2, 2018

View reviewed changes

Cleaned up return values to always be an array

f529d9a

Building an array of node ids instead of functions

ee72d87

cpettitt approved these changes Feb 6, 2018

View reviewed changes

cpettitt merged commit c6cacf7 into dagrejs:master Feb 6, 2018

cpettitt mentioned this pull request Feb 6, 2018

maximum call stack size exceeded when my graph is large #223

Closed

jart mentioned this pull request Mar 31, 2018

Remove Object.values() tensorflow/tensorboard#1089

Merged

dhess mentioned this pull request May 25, 2022

feat: Add a tree-rendering component using React Flow hackworthltd/primer-app#339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pass1 and pass2 no longer cause a stack overflow for large graphs #228

pass1 and pass2 no longer cause a stack overflow for large graphs #228

rustedgrail commented Jan 24, 2018

miriammelnick left a comment

miriammelnick Jan 24, 2018

rustedgrail Jan 24, 2018

miriammelnick Jan 24, 2018

rustedgrail Jan 24, 2018

miriammelnick Jan 24, 2018

rustedgrail Jan 24, 2018

miriammelnick Jan 24, 2018

rustedgrail Jan 24, 2018

miriammelnick Jan 24, 2018

rustedgrail Jan 24, 2018

miriammelnick Jan 24, 2018

miriammelnick Jan 24, 2018

rustedgrail Jan 24, 2018

miriammelnick Jan 24, 2018

rustedgrail Jan 24, 2018

lutzroeder commented Jan 25, 2018

miriammelnick commented Jan 25, 2018

lutzroeder commented Jan 25, 2018 •

edited

rustedgrail commented Jan 30, 2018

lutzroeder commented Jan 30, 2018 •

edited

rustedgrail commented Jan 31, 2018 •

edited

cpettitt left a comment

rustedgrail commented Feb 2, 2018

cpettitt commented Feb 3, 2018

rustedgrail commented Feb 5, 2018

cpettitt commented Feb 6, 2018

pass1 and pass2 no longer cause a stack overflow for large graphs #228

pass1 and pass2 no longer cause a stack overflow for large graphs #228

Conversation

rustedgrail commented Jan 24, 2018

miriammelnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lutzroeder commented Jan 25, 2018

miriammelnick commented Jan 25, 2018

lutzroeder commented Jan 25, 2018 • edited

rustedgrail commented Jan 30, 2018

lutzroeder commented Jan 30, 2018 • edited

rustedgrail commented Jan 31, 2018 • edited

cpettitt left a comment

Choose a reason for hiding this comment

rustedgrail commented Feb 2, 2018

cpettitt commented Feb 3, 2018

rustedgrail commented Feb 5, 2018

cpettitt commented Feb 6, 2018

lutzroeder commented Jan 25, 2018 •

edited

lutzroeder commented Jan 30, 2018 •

edited

rustedgrail commented Jan 31, 2018 •

edited