Bug 101085 – Web Inspector: Fix heap snapshots counted several times by NMI

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

[Alexei Filippov]

Created attachment 172105 [details]
screenshot

Heap snapshots data size is counted twice. See the attached screenshot. The heap snapshot data is actually takes 208MB, but NMI reports 416MB.
To reproduce just open the profiler, take a heap snapshot and look at the native memory chart.

[Alexei Filippov]

Created attachment 172107 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 172107 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Web Inspector: Fix heap snapshots counted several times by NMI
> +        https://bugs.webkit.org/show_bug.cgi?id=101085

It is not clear for me what was the case.

[Ilya Tikhonovsky]

(In reply to comment #2)
> (From update of attachment 172107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172107&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        Web Inspector: Fix heap snapshots counted several times by NMI
> > +        https://bugs.webkit.org/show_bug.cgi?id=101085
> 
> It is not clear for me what was the case.

I mean what was the mechanic of this double counting.

[Yury Semikhatsky]

Comment on attachment 172107 [details]
Patch

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

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:510
> +    static ScriptProfilerSingletonProxy scriptProfilerSingletonProxy;

static variable will not work for workers.

[Alexei Filippov]

Comment on attachment 172107 [details]
Patch

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

>>> Source/WebCore/ChangeLog:4
>>> +        https://bugs.webkit.org/show_bug.cgi?id=101085
>> 
>> It is not clear for me what was the case.
> 
> I mean what was the mechanic of this double counting.

There are two instances of InspectorProfilerAgent, so addPrivateBuffer is called twice.

>> Source/WebCore/inspector/InspectorProfilerAgent.cpp:510
>> +    static ScriptProfilerSingletonProxy scriptProfilerSingletonProxy;
> 
> static variable will not work for workers.

Sorry, could you please elaborate. Why not?

[Yury Semikhatsky]

Comment on attachment 172107 [details]
Patch

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

>>> Source/WebCore/inspector/InspectorProfilerAgent.cpp:510
>>> +    static ScriptProfilerSingletonProxy scriptProfilerSingletonProxy;
>> 
>> static variable will not work for workers.
> 
> Sorry, could you please elaborate. Why not?

The profiler agent can belong to a dedicated worker that lives on its own thread and ScriptProfilerSingletonProxy can be called concurrently.

[Alexei Filippov]

Comment on attachment 172107 [details]
Patch

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

>>>> Source/WebCore/inspector/InspectorProfilerAgent.cpp:510
>>>> +    static ScriptProfilerSingletonProxy scriptProfilerSingletonProxy;
>>> 
>>> static variable will not work for workers.
>> 
>> Sorry, could you please elaborate. Why not?
> 
> The profiler agent can belong to a dedicated worker that lives on its own thread and ScriptProfilerSingletonProxy can be called concurrently.

Yes, but this class is an empty POD that has neither constructor nor destructor. I think empty functions can be considered thread safe. I don't want to use DEFINE_STATIC_LOCAL here because in this case its initialization with a heap object may cause a real race condition leading two objects being allocated. I can see hundreds of function static data used across WebKit without DEFINE_STATIC_LOCAL.

[Alexei Filippov]

Created attachment 172829 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 172829 [details]
Patch

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

> Source/WebCore/bindings/v8/V8PerIsolateData.cpp:128
> +
> +    ScriptProfilerInstrumentationProxy scriptProfilerInstrumentationProxy;
> +    info.addMember(scriptProfilerInstrumentationProxy);

it looks like a hack.
I'd rather add objectType parameter with default value 0 to addPrivateBuffer method.

[Yury Semikhatsky]

(In reply to comment #9)
> (From update of attachment 172829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172829&action=review
> 
> > Source/WebCore/bindings/v8/V8PerIsolateData.cpp:128
> > +
> > +    ScriptProfilerInstrumentationProxy scriptProfilerInstrumentationProxy;
> > +    info.addMember(scriptProfilerInstrumentationProxy);
> 
> it looks like a hack.
> I'd rather add objectType parameter with default value 0 to addPrivateBuffer method.

Sounds like the second approach we discussed with Alexei, let's implement it instead.

[Yury Semikhatsky]

Comment on attachment 172829 [details]
Patch

r- per Ilya's comment

[Alexei Filippov]

Created attachment 173022 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173022 [details]
Patch

Clearing flags on attachment: 173022

Committed r134033: <http://trac.webkit.org/changeset/134033>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.