Bug 136380 – LegacyProfiler: remove redundant ProfileNode members and other cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Brian Burg]

looks like dead code to me, since the frontend sums call data itself. This can be explicitly computed in debug logging code.

[Brian Burg]

Created attachment 237418 [details]
Patch

[Timothy Hatcher]

Comment on attachment 237418 [details]
Patch

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

> Source/JavaScriptCore/profiler/ProfileNode.h:145
> -        Vector<Call, 1> m_calls;
> +        Vector<Call> m_calls;

Why this change? There will always be a minimum of 1 call, so it seems best to have storage for it instead of another malloc right away.

[Joseph Pecoraro]

Comment on attachment 237418 [details]
Patch

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

> Source/WebCore/inspector/ScriptProfile.idl:33
> -    readonly attribute ScriptProfileNode head;
> -    readonly attribute unrestricted double idleTime;
> +    readonly attribute ScriptProfileNode rootNode;

Will we need to change LegacyJavaScriptProfileView.js which still makes use of "profileNode.head" which no longer exists?

[Joseph Pecoraro]

(In reply to comment #3)
> (From update of attachment 237418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237418&action=review
> 
> > Source/WebCore/inspector/ScriptProfile.idl:33
> > -    readonly attribute ScriptProfileNode head;
> > -    readonly attribute unrestricted double idleTime;
> > +    readonly attribute ScriptProfileNode rootNode;
> 
> Will we need to change LegacyJavaScriptProfileView.js which still makes use of "profileNode.head" which no longer exists?

Maybe this was only used by tests?

[Brian Burg]

Comment on attachment 237418 [details]
Patch

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

>> Source/JavaScriptCore/profiler/ProfileNode.h:145
>> +        Vector<Call> m_calls;
> 
> Why this change? There will always be a minimum of 1 call, so it seems best to have storage for it instead of another malloc right away.

For some reason, the compiler couldn't figure out how to pass a reference to m_calls if it had the default inline size. Maybe Vector was forward-declared instead of defined? I'll try again.

>>> Source/WebCore/inspector/ScriptProfile.idl:33
>>> +    readonly attribute ScriptProfileNode rootNode;
>> 
>> Will we need to change LegacyJavaScriptProfileView.js which still makes use of "profileNode.head" which no longer exists?
> 
> Maybe this was only used by tests?

The tests were changed to use rootNode too. My understanding is that the legacy profiler views are only used when the inspected page speaks a legacy protocol. But, I have no way of testing this. Tim?

[Timothy Hatcher]

Comment on attachment 237418 [details]
Patch

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

> Source/JavaScriptCore/profiler/ProfileNode.h:99
> -        Vector<Call> calls() const { return m_calls; }
> +        const Vector<Call>& calls() const { return m_calls; }

Oh, I see. This is what required the change of no default size. Hmm...

[Timothy Hatcher]

LegacyJavaScriptProfile*.{js,css} can be removed. I punted on making it work after the profile migration to Timeline.

[Brian Burg]

Committed r173199: <http://trac.webkit.org/changeset/173199>