[webkit-reviews] review granted: [Bug 196890] Web Inspector: Heap: don't use recursion when calculating root paths : [Attachment 367366] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 196890: Web Inspector: Heap: don't use recursion when calculating root
paths
https://bugs.webkit.org/show_bug.cgi?id=196890

Attachment 367366: Patch

https://bugs.webkit.org/attachment.cgi?id=367366&action=review




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 367366
  --> https://bugs.webkit.org/attachment.cgi?id=367366
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367366&action=review

r=me

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:756
> +	   for (let i = 0; i < stack.length; ++i) {
> +	       let {currentPath, nodeOrdinal} = stack[i];

This list is not being treated as a stack. In fact it is always growing because
nothing nothing is ever removed.

Normally when we have a stack approach like this we process it to exhaustion
like this:

    let stack = [{...entry...}];
    while (stack.length) {
	let entry = stack.pop();
	// Process entry, maybe push new stack entries.
    }

Which I think could be done here as long as you convert these two lines to:

    while (stack.length) {
	let {currentPath, nodeOridinal} = stack.pop();
	...

That said, items are going to be processes on the stack in reverse order... so
to get unchanging output you could iterate the edges in forward order such as:

    while (stack.length) {
	let {currentPath, nodeOridinal} = stack.unshift();
	...

That should get you output that is consistent with the current code.