Bug 55794 – Web Inspector: Need new graphic icon for garbage collect button

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

[Greg Simon]

Created attachment 84802 [details]
patch adding gc button with temporary graphic icon

Attached is a patch that will add an explicit garbage collect button to the timeline (both v8 and JSC).  Often when diagnosing memory issues with web applications it is useful to know if the accumulated memory in the heap is flushable or not.  By clicking on this button you get a forced  collection so you do not have to wait for it to open automatically.

We need a graphic image for it -- I made an attempt as a placeholder but it looks more like a grande coffee cup (not the intention!)

[Greg Simon]

Created attachment 84803 [details]
A picture of the feature running with the temporary graphic button.

[Greg Simon]

Created attachment 84804 [details]
The temporary icon pulled out of the also attached patch (grande coffee cup)

[Mikhail Naganov]

Looks good to me except for indentations. Please run Tools/Scripts/check-webkit-style.

[Pavel Feldman]

Comment on attachment 84802 [details]
patch adding gc button with temporary graphic icon

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

This looks good except for a bunch of nits and a lack of test. You could add one into LayoutTests/inspector/timeline/(timeline-gc.html)? You can't use timeline runner harness as is since your action is triggered by the front-end, but you should do a manual timeline test. For that:

1. copy timeline-script-tab-1.html
2. nuke performActions
3. Test would look like:

function test() 
{
    InspectorTest.startTimeline(step1);

    function step1()
    {
        ProfilerAgent.collectGarbage(step2);
    }

    function step2()
    {
        InspectorTest.stopTimeline(step3);
        InspectorTest.printTimelineRecords();
    }

    function step3()
    {
        InspectorTest.printTimelineRecords();
        InspectorTest.completeTest();
    }
}

You will most likely end up with flaky parameters / entries in the timeline dump, you can use InspectorTest.printTimelineRecords's filter / formatter as in timeline-script-tag-1.html to resolve that.

> Source/WebCore/bindings/js/ScriptProfiler.cpp:34
> +#include "GCController.h"

Nit: mind alphabetic order.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:47
> +#include "ScriptController.h"

Alphabetic order please.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:48
> +#include "Frame.h"

Is this import needed?

> Source/WebCore/inspector/front-end/Images/garbageCollectButtonGlyph.png:1
> +PNG

What generated this patch? Usually webkit-patch, svn-patch or git diff --binary results in image previews available in bugzilla.

> Source/WebCore/inspector/front-end/TimelinePanel.js:212
> +        this.garbageCollectButton = new WebInspector.StatusBarButton(WebInspector.UIString("Collect Garbage"), "garbage-collect-status-bar-item");

You need to add this string to WebCore/English.lproj/localizedStrings.js (watch out, utf-16 binary file).

> Source/WebCore/inspector/front-end/TimelinePanel.js:293
> +    	ProfilerAgent.collectGarbage();

Weird indentation.

[Pavel Feldman]

We were discussing adding counters into the Node (and other classes) the other day. Now imagine there are counters and we are sending them with every timeline event and we visualize their graphs as we do with memory.

But you need to force gc in order for these counters to make sense. As a result, in addition to the button in the UI you need a programmatic was of performing gc from a web-facing API. Exposing console.gc that would work while inspector is opened is an option, performance.memory.gc() that would work whenever performance.memory is exposed would also work. As a result, I think we should:

1) expose performance.memory.gc()
2) make performance.memory work whenever inspector for the page is opened

both seem trivial and for good. This can be a part of a separate change of course.

[Greg Simon]

The object counters are a useful tool -- the ones that I've found the most useful when diagnosing mem leaks in webkit are


- Node -- catches JS errors where DOM Nodes are increasingly created

- EventListener -- catches cases where JS code keeps adding event listeners to nodes. Also provides a starting point to analyze closures that may be keeping bits around via JS heap graph.

- Document -- ref counting bugs will eventually cause these to stick around. This one is useful to C++ devleopers but probably not JS folks since there should not be an ability for JS code to cause this to happen. However it still is useful and revealing to see how long these Document instances can stick around usually due to GC and JS/webcore binding behavior.

[Pavel Feldman]

(In reply to comment #6)
> The object counters are a useful tool -- the ones that I've found the most useful when diagnosing mem leaks in webkit are
> 
> 
> - Node -- catches JS errors where DOM Nodes are increasingly created
> 
> - EventListener -- catches cases where JS code keeps adding event listeners to nodes. Also provides a starting point to analyze closures that may be keeping bits around via JS heap graph.
> 
> - Document -- ref counting bugs will eventually cause these to stick around. This one is useful to C++ devleopers but probably not JS folks since there should not be an ability for JS code to cause this to happen. However it still is useful and revealing to see how long these Document instances can stick around usually due to GC and JS/webcore binding behavior.

One can surely hold many documents from within javascript via say element's style property, so I think this is useful for JavaScript developers as well.

I look at Node::dumpStatistics (http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Node.cpp&q=Node.h&exact_package=chromium&d=6&l=135) and I don't see why we can't move most of its goodness from behind the flag. Putting static counters into various kinds of nodes should not be expensive. We could also keep track of the number of attached nodes via tracking setDocument. Not a strict requirement - they are anyways reachable and hence countable, but seeing detached DOM numbers is handy and is also inexpensive.

[Mikhail Naganov]

(In reply to comment #5)
> We were discussing adding counters into the Node (and other classes) the other day. Now imagine there are counters and we are sending them with every timeline event and we visualize their graphs as we do with memory.
> 
> But you need to force gc in order for these counters to make sense. As a result, in addition to the button in the UI you need a programmatic was of performing gc from a web-facing API. Exposing console.gc that would work while inspector is opened is an option, performance.memory.gc() that would work whenever performance.memory is exposed would also work. As a result, I think we should:
> 
> 1) expose performance.memory.gc()
> 2) make performance.memory work whenever inspector for the page is opened
> 

2) already works, it doesn't need Inspector. "performance.memory" is already always exposed, it contains zero values if reporting is disabled.

> both seem trivial and for good. This can be a part of a separate change of course.

[Greg Simon]

Created attachment 85019 [details]
Updated patch addressing issues from first review/

Graphic icon provided was created by author.

[Greg Simon]

Created attachment 85021 [details]
Updated patch addressing issues from first review

Includes new icon drawn by patch author.

[Pavel Feldman]

Comment on attachment 85021 [details]
Updated patch addressing issues from first review

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

Heh, I don't think we can land with this icon :)

> LayoutTests/inspector/timeline/timeline-gc-button-expected.txt:1
> +Tests the Timeline Garbage Collect button.

So it clearly did not work - otherwise you would have seen timeline GC event in the log.

> LayoutTests/inspector/timeline/timeline-gc-button.html:19
> +        InspectorTest.printTimelineRecords();

I don't think we need to print timeline records twice.

[Greg Simon]

Created attachment 85290 [details]
Updated attachment removing flaky unit test, reverting icon to latte cup (see comment)

It is difficult to do a unit test for this for the following reasons:

- We need a dedicated v8 API to do a collection -- right now this patch callsV8::LowMemoryNotification() whose implementation could change (result: flaky test)
REMEDY: add new v8 public API for explicitly doing a single collection

- JSC currently does not report GC events on the timeline (not plumbed out) so this button runs open-loop
REMEDY: I have opened up a bug for this: https://bugs.webkit.org/show_bug.cgi?id=56070

[Pavel Feldman]

Comment on attachment 85290 [details]
Updated attachment removing flaky unit test, reverting icon to latte cup (see comment)

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

> Source/WebCore/ChangeLog:4
> +

Bug title is missing. I'll add one from the bug and land it for you.

[Pavel Feldman]

Committed r80723: <http://trac.webkit.org/changeset/80723>