Bug 61178 – Web Inspector: [Chromium] Add an ability to show the objects that were allocated between snapshot N-2 and snapshot N-1 and still alive in snapshot N

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Mikhail Naganov]

This is very helpful for identifying leaked objects left from operations.

From Michael Davidson <[e-mail]>, leak discovery scenario for GMail:

"
1) Take a "baseline"
2) Navigate to compose
3) Take a snapshot. Call the difference between this snapshot and the baseline "the delta".
4) Discard the compose.
5) Take a final snapshot

Now I want to be able to see the stuff that was in "the delta" that is still on the heap, but without any of the stuff that was created between steps 3 and 5. Does that make sense? In other words, I want everything allocated between step 1 and step 3 to be conceptually in a separate heap that I can examine on its own at any time without seeing objects that were allocated before step 1 or after step 3.
"

This can be solved by implementing an ability to intersect and subtract an arbitrary amount of snapshots.
Thus, if you have snapshots S1, S3 and S5 corresponding to steps 1, 3
and 5 of your scenario, you'll need to perform (S3 - S1) / S5  ("/"
means intersect), to leave only the objects that were allocated
between steps 1 and 3, and are currently on the heap.

[Ilya Tikhonovsky]

I thi
It will be an additional view for Heap snapshot.
The idea of this view is to show all the objects of Snapshot N which were allocated at Snapshot N-1, or N-2 etc

[Ilya Tikhonovsky]

Created attachment 111132 [details]
Patch

[Pavel Feldman]

(In reply to comment #1)
> It will be an additional view for Heap snapshot.
> The idea of this view is to show all the objects of Snapshot N which were allocated at Snapshot N-1, or N-2 etc

I am not sure whether this is the best way of addressing it. I'd like to see some user scenarios / success stories proving that this is the best / flexible enough way to address the original request. Rationale: profiler UI is already way too complex + this change introduces more UI -> more complexity.

If we were to formulate this problem in the version control terms, the solution would be "three-way-merge":

S1 = baseline
S3 = change 'delta' applied
S5 = change 'delta' partially reverted, some more changes introduced

No we need to find out what part of change 'delta' was not reverted.

[Pavel Feldman]

Comment on attachment 111132 [details]
Patch

Removing r? until we decide the solution is what we need.

[Ilya Tikhonovsky]

For solving this problem we can operate (In reply to comment #3)
> (In reply to comment #1)
> > It will be an additional view for Heap snapshot.
> > The idea of this view is to show all the objects of Snapshot N which were allocated at Snapshot N-1, or N-2 etc
> 
> I am not sure whether this is the best way of addressing it. I'd like to see some user scenarios / success stories proving that this is the best / flexible enough way to address the original request. Rationale: profiler UI is already way too complex + this change introduces more UI -> more complexity.
> 
> If we were to formulate this problem in the version control terms, the solution would be "three-way-merge":
> 
> S1 = baseline
> S3 = change 'delta' applied
> S5 = change 'delta' partially reverted, some more changes introduced
> 
> Now we need to find out what part of change 'delta' was not reverted.

We have a monotonically increasing identifiers for the objects in the heap. I've asked mnaganov at Friday. He implemented a map between ids and objects in v8 code. He also updates the map at GC stage.

1) S1 is a set of objects with identifiers in range 1-1000
2) S3 is a subset of objects from S1 and a set of new objects. These new objects got identifiers in range 1001-2000.
3) S5 is a subset of objects from S1 (probably smaller than such subset in S3), a subset of other objects from S3 (probably smaller than it was in S3, it is a leak) and a set of new objects. These new objects got identifiers in range 2001-3000.

We want to catch a leak. It is pretty simple. We should print the objects from S5 which have identifiers in range [1001 - 2000]

I can do that as three way merge but it will be a bit slow :)

[Mikhail Naganov]

Comment on attachment 111132 [details]
Patch

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

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:233
> +WebInspector.HeapSnapshotLeaksDataGrid.prototype = {

I'm also against introducing a new grid. We should the comparison view to be more useful.

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:278
> +        var key = this._minNodeId + ".." + this._maxNodeId;

I think you can use filter source as a key. You seem to be only using key as a unique id, without taking advantage of the fact that it is constructed from min and max ids.

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:323
> +    _baseProfileIndexChanged: function(loader, profileIndex)

Current implementation interferes with the comparison view. Try this: having two snapshots, make the 2nd active, then select "Allocated at Snapshot 1", then change "Allocated at" to "Comparison" -- nothing will happen, because the comparison view will not be triggered.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:744
> +            if ((id % 2) && id > this._maxNodeId)

(id % 2) should be a function with a meaningful name.

[Mikhail Naganov]

(In reply to comment #6)
> (From update of attachment 111132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111132&action=review
> 
> > Source/WebCore/inspector/front-end/DetailedHeapshotView.js:233
> > +WebInspector.HeapSnapshotLeaksDataGrid.prototype = {
> 
> I'm also against introducing a new grid. We should the comparison view to be more useful.
> 
> > Source/WebCore/inspector/front-end/DetailedHeapshotView.js:278
> > +        var key = this._minNodeId + ".." + this._maxNodeId;
> 
> I think you can use filter source as a key. You seem to be only using key as a unique id, without taking advantage of the fact that it is constructed from min and max ids.
> 
> > Source/WebCore/inspector/front-end/DetailedHeapshotView.js:323
> > +    _baseProfileIndexChanged: function(loader, profileIndex)
> 
> Current implementation interferes with the comparison view. Try this: having two snapshots, make the 2nd active, then select "Allocated at Snapshot 1", then change "Allocated at" to "Comparison" -- nothing will happen, because the comparison view will not be triggered.
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:744
> > +            if ((id % 2) && id > this._maxNodeId)
> 
> (id % 2) should be a function with a meaningful name.

(In reply to comment #4)
> (From update of attachment 111132 [details])
> Removing r? until we decide the solution is what we need.

I have an idea for visualization of snapshots operations: we can represent a snapshot as a strip with marks representing object ids. Like this:

 [      ||       |    #        |||  #                  ]
 [      ||       |    #        |||  #                  ]
 0  prev. generations # current gen.#            Current Max ID

This will make a snapshot look similar to how disk contents are represented in defragmentation software. This way, intersections and identifier ranges will look quite intuitive.

So, on the leaks pane we can arrange several snapshots as strips and allow drag and drop or something similar for acting on them:

 Snapshot 1 [#  | |  |    #                      ]
 Snapshot 2 [   |    |    # |  |  | |  #         ]
 Snapshot 3 [   |              |  |    # | | #   ]
             previous generations      # current

[Ilya Tikhonovsky]

Created attachment 111280 [details]
Patch

[WebKit Review Bot]

Attachment 111280 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   7d79b00..04b6eb5  master     -> origin/master
	M	ChangeLog
	M	Source/autotools/symbols.filter
r97631 = 04b6eb5a64bd2f7d8671d6f60f1a541071161d9b (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Tikhonovsky]

Created attachment 111394 [details]
screenshot

[Mikhail Naganov]

Comment on attachment 111280 [details]
Patch

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

I like this change. I think we may get rid of the Comparison view after submitting this one. This one works faster and provides a simpler but still detailed enough view.

> Source/WebCore/ChangeLog:11
> +        4) select last snapshot and choose 'Allocated at' view for it.

Is this still true?

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:264
> +            this._minNodeId = snapshot.maxNodeId;

Shouldn't it be snapshot.maxNodeId + 2? Or better, extract out Id logic into a separate helper class, together with (id % 2) expression in maxNodeId, so it will be Helper.nextNodeId(snapshot.maxNodeId)

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1275
> +            filterOption.label = "All objects";

WebInspector.UIString

> Source/WebCore/inspector/front-end/HeapSnapshot.js:763
> +            if ((id % 2) && id > this._maxNodeId)

Please extract "id % 2" into a separate class dealing with Ids.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:817
> +    filteredAggregates: function(key, filter)

Can we make "unfiltered" aggregates a special case of filtered, with an empty filter?

> Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:410
> +        setTimeout(callback.bind(null, this), 0);

Why do you need this?

[Ilya Tikhonovsky]

Created attachment 111581 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #11)
> (From update of attachment 111280 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111280&action=review
> 
> I like this change. I think we may get rid of the Comparison view after submitting this one. This one works faster and provides a simpler but still detailed enough view.
> 
> > Source/WebCore/ChangeLog:11
> > +        4) select last snapshot and choose 'Allocated at' view for it.
> 
> Is this still true?

done.



> 
> > Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1275
> > +            filterOption.label = "All objects";
> 
> WebInspector.UIString

done

> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:763
> > +            if ((id % 2) && id > this._maxNodeId)
> 
> Please extract "id % 2" into a separate class dealing with Ids.

I'd like to move this calculation to backend code in another change. It will cost us constant complexity time instead linear.  


> 
> > Source/WebCore/inspector/front-end/DetailedHeapshotView.js:264
> > +            this._minNodeId = snapshot.maxNodeId;
> 
> Shouldn't it be snapshot.maxNodeId + 2? Or better, extract out Id logic into a separate helper class, together with (id % 2) expression in maxNodeId, so it will be Helper.nextNodeId(snapshot.maxNodeId)

After moving maxNodeId calculation to the backend the magic with '+ 2' will looks weird.
I just check half-closed interval for the ids like (min, max]

> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:817
> > +    filteredAggregates: function(key, filter)
> 
> Can we make "unfiltered" aggregates a special case of filtered, with an empty filter?

done


> 
> > Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:410
> > +        setTimeout(callback.bind(null, this), 0);
> 
> Why do you need this?

it was a bug. If the snapshot is not loaded yet then HeapSnapshotLoadingProxy loads the snapshot and passes it as an argument to the callback.
In the other case HeapShapshotProxy calls the callback immediately but with no arguments.

[Mikhail Naganov]

Comment on attachment 111581 [details]
Patch

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

Nice!

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1286
> +                    title = WebInspector.UIString("Objects allocated between Snapshot %d and Snapshot %d", title.substring(UserInitiatedProfileName.length + 1) - 1, title.substring(UserInitiatedProfileName.length + 1));

Please consider shortening to "Objects allocated between Snapshots %d and %d"

> Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:96
> +    this._worker = new WebInspector.HeapSnapshotFakeWorker();

I believe this change is accidental

[Ilya Tikhonovsky]

Created attachment 111593 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #14)
> (From update of attachment 111581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111581&action=review
> 
> Nice!
> 
> > Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1286
> > +                    title = WebInspector.UIString("Objects allocated between Snapshot %d and Snapshot %d", title.substring(UserInitiatedProfileName.length + 1) - 1, title.substring(UserInitiatedProfileName.length + 1));
> 
> Please consider shortening to "Objects allocated between Snapshots %d and %d"

done

> 
> > Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:96
> > +    this._worker = new WebInspector.HeapSnapshotFakeWorker();
> 
> I believe this change is accidental

done

[Mikhail Naganov]

Comment on attachment 111593 [details]
Patch

lgtm!

[Ilya Tikhonovsky]

Committed r97855: <http://trac.webkit.org/changeset/97855>