Bug 136380 - LegacyProfiler: remove redundant ProfileNode members and other cleanup
Summary: LegacyProfiler: remove redundant ProfileNode members and other cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords:
Depends on: 136462
Blocks: 136352 136381
  Show dependency treegraph
 
Reported: 2014-08-29 12:10 PDT by Brian Burg
Modified: 2014-09-02 21:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (39.43 KB, patch)
2014-08-30 16:42 PDT, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-08-29 12:10:35 PDT
looks like dead code to me, since the frontend sums call data itself. This can be explicitly computed in debug logging code.
Comment 1 Brian Burg 2014-08-30 16:42:34 PDT
Created attachment 237418 [details]
Patch
Comment 2 Timothy Hatcher 2014-09-02 10:59:53 PDT
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.
Comment 3 Joseph Pecoraro 2014-09-02 11:05:51 PDT
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?
Comment 4 Joseph Pecoraro 2014-09-02 11:06:12 PDT
(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?
Comment 5 Brian Burg 2014-09-02 11:09:51 PDT
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?
Comment 6 Timothy Hatcher 2014-09-02 11:13:08 PDT
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...
Comment 7 Timothy Hatcher 2014-09-02 11:52:03 PDT
LegacyJavaScriptProfile*.{js,css} can be removed. I punted on making it work after the profile migration to Timeline.
Comment 8 Brian Burg 2014-09-02 21:59:05 PDT
Committed r173199: <http://trac.webkit.org/changeset/173199>