misc: drive-by code cleanup in BaseNode #7723

brendankenny · 2019-03-26T20:10:44Z

While starting work on robust TTI, there were some things that were a little hard to follow as I was stepping through graphs. Tried to make them a bit clearer in code and comments while keeping everything working the same.

@patrickhulce let me know if this feels like too much change for change's sake and I can scale it back :)

brendankenny · 2019-03-26T20:15:28Z

slightly better diff with ?w=1

brendankenny · 2019-03-26T20:16:31Z

lighthouse-core/lib/dependency-graph/base-node.js

-        if (predicate(node)) {
-          node.traverse(
-            node => idsToInclude.add(node.id),
-            node => node._dependencies.filter(parent => !idsToInclude.has(parent))


dangers of Set<any>...we were checking the parent node object against a set of string IDs, so weren't saving any time here :)

I guess this one is difficult to test since it's just a marginal time savings huh

I guess this one is difficult to test since it's just a marginal time savings huh

yeah, and even at its worst it's really not that bad compared to other things we do with the graph

patrickhulce

nice looks pretty good to me! just the bit about skipping some cloning when there's not a predicate

patrickhulce · 2019-03-26T20:41:38Z

lighthouse-core/lib/dependency-graph/base-node.js

+      // Walk back up dependencies, cloning all nodes in the path to this one.
+      node.traverse(
+        node => idsToIncludedClones.set(node.id, node.cloneWithoutRelationships()),
+        node => node._dependencies.filter(parent => !idsToIncludedClones.has(parent.id))


can we get a comment on this one that explains the filter while we're at it? :) it even took me a sec to remember why this was happening at all

can we get a comment on this one that explains the filter while we're at it?

good call

patrickhulce · 2019-03-26T20:43:39Z

lighthouse-core/lib/dependency-graph/base-node.js

+      if (idsToIncludedClones.has(node.id)) return;
+      if (predicate && !predicate(node)) return;
+
+      // Walk back up dependencies, cloning all nodes in the path to this one.


can we only do the reverse traversal when there's a predicate?

that's one nice feature of the previous impl I wouldn't want to lose

can we only do the reverse traversal when there's a predicate?

do you mean something like

// Walk down dependents. rootNode.traverse(node => { if (idsToIncludedClones.has(node.id)) return; if (!predicate) { // Clone every node. idsToIncludedClones.set(node.id, node.cloneWithoutRelationships()); } else if (predicate(node)) { // Walk back up dependencies, cloning all nodes in the path to this one. node.traverse( node => idsToIncludedClones.set(node.id, node.cloneWithoutRelationships()), // Dependencies already cloned have already cloned ancestors, so no need to visit again. node => node._dependencies.filter(parent => !idsToIncludedClones.has(parent.id)) ); } });

Or other ideas for formatting to make it less dense looking?

Another option is to make a default always true predicate, so it's a little clearer.

Agreed it is a bit weird to do the reverse traversal in the no predicate case. My thinking before was that we only have a single call of cloneWithRelationships without a predicate, so it didn't seem to be a super important case to call out (and since the filter line makes there be no next nodes for every node, it doesn't do any significant extra work)

that's exactly what I meant :)

we could do an early return in the !predicate case to avoid the nesting? but IMO this is fine

patrickhulce · 2019-03-26T20:46:31Z

lighthouse-core/lib/dependency-graph/base-node.js

-      getNext = node => node.getDependents();
-    }
+    const visited = new Set([this.id]);
+    const queue = [[/** @type {Node} */(/** @type {BaseNode} */(this))]];


do we still need this? I'm surprised we need to cast BaseNode for this hopefully this is something that was fixed :)

do we still need this?

Annoyingly, yes. this extends BaseNode, but the lie is that it's a Node. Humans know the only types of BaseNode are CPUNode | NetworkNode so the cast to Node is ok, but the compiler accounts for the fact that we could be calling on a different class derived from BaseNode, so it's not necessarily compatible with Node.

So to cast while following the type assertion rules, we have to upcast to BaseNode and then downcast to Node. The other options is an assertion and a @ts-ignore...maybe that's more honest (albeit also ugly).

Humans know the only types of BaseNode are CPUNode | NetworkNode so

I mean, even that's not entirely true. In tests we call traverse() with a graph of BaseNode nodes, but in real code it's much more convenient to only get CPUNode | NetworkNode back, so we should continue to pretend it's true :)

patrickhulce · 2019-03-26T20:47:15Z

lighthouse-core/lib/dependency-graph/base-node.js

   */
-  _traversePaths(iterator, getNext) {
-    const stack = [[/** @type {Node} */(/** @type {BaseNode} */(this))]];


lol I guess this is a queue :)

lol I guess this is a queue :)

your comment in hasCycle (which does DFS) underlined the humor:

// Get the last node in the stack (DFS uses stack, not queue)

I was thinking the approach changed at some point during the writing of traverse :)

haha yeah that was my guess as well when I was looking at this, I'll take your benefit of the doubt on this one lol 🤞

patrickhulce · 2019-03-26T20:49:04Z

lighthouse-core/lib/dependency-graph/base-node.js

-      return nodesToVisit;
-    };
+      for (const nextNode of getNextNodes(node)) {
+        if (visited.has(nextNode.id)) continue;


having trouble finding the substantial changes here, it's just inlining the getNext rewrite from before and eliminating traverse right?

it's just inlining the getNext rewrite from before and eliminating _traversePaths right?

yeah, main thing was the getNext rewriting and call to _traversePaths was breaking my brain when I was first (re)reading it, and it seemed like traverse has proven its design, so it seemed ok to inline at this point.

This was probably the bigger one for me saying it's ok to push back on changes if you don't think it reads better :)

technically we save n-1 visited.add(node.id) calls, but that's not worth much and could have been done without all the other things.

nah, I like this. originally I had used _traversePaths in another method but that need hasn't materialized in the 1.5 years since so I think we're pretty safe :)

patrickhulce

LGTM!

brendankenny added 2 commits March 26, 2019 13:02

misc: driveby code cleanup in BaseNode

f5edbe9

add traversal path test

706edcd

brendankenny requested a review from paulirish as a code owner March 26, 2019 20:10

brendankenny changed the title ~~misc: drive-by code cleanup in BaseNode~~ misc: drive by code cleanup in BaseNode Mar 26, 2019

brendankenny changed the title ~~misc: drive by code cleanup in BaseNode~~ misc(base-node): drive-by code cleanup Mar 26, 2019

brendankenny changed the title ~~misc(base-node): drive-by code cleanup~~ misc: drive-by code cleanup in BaseNode Mar 26, 2019

brendankenny commented Mar 26, 2019

View reviewed changes

patrickhulce reviewed Mar 26, 2019

View reviewed changes

brendankenny added 2 commits March 26, 2019 18:05

most of the feedback

4ac3360

predicated

6159fcc

patrickhulce approved these changes Mar 27, 2019

View reviewed changes

brendankenny merged commit 21dda6a into master Mar 28, 2019

brendankenny deleted the iter branch March 28, 2019 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: drive-by code cleanup in BaseNode #7723

misc: drive-by code cleanup in BaseNode #7723

brendankenny commented Mar 26, 2019 •

edited

Loading

brendankenny commented Mar 26, 2019

brendankenny Mar 26, 2019 •

edited

Loading

patrickhulce Mar 26, 2019

patrickhulce Mar 26, 2019

brendankenny Mar 26, 2019

patrickhulce left a comment

patrickhulce Mar 26, 2019

brendankenny Mar 26, 2019

patrickhulce Mar 26, 2019

brendankenny Mar 27, 2019

patrickhulce Mar 27, 2019

brendankenny Mar 27, 2019

patrickhulce Mar 26, 2019

brendankenny Mar 26, 2019

brendankenny Mar 26, 2019

patrickhulce Mar 26, 2019

brendankenny Mar 26, 2019

patrickhulce Mar 27, 2019

patrickhulce Mar 26, 2019

brendankenny Mar 26, 2019

patrickhulce Mar 27, 2019

patrickhulce left a comment

misc: drive-by code cleanup in BaseNode #7723

misc: drive-by code cleanup in BaseNode #7723

Conversation

brendankenny commented Mar 26, 2019 • edited Loading

brendankenny commented Mar 26, 2019

brendankenny Mar 26, 2019 • edited Loading

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

patrickhulce 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

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny commented Mar 26, 2019 •

edited

Loading

brendankenny Mar 26, 2019 •

edited

Loading