Bug 136380

Summary: LegacyProfiler: remove redundant ProfileNode members and other cleanup
Product: WebKit Reporter: Brian Burg <burg>
Component: JavaScriptCoreAssignee: Brian Burg <burg>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, mark.lam, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 136462    
Bug Blocks: 136352, 136381    
Attachments:
Description Flags
Patch timothy: review+

Brian Burg
Reported 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.
Attachments
Patch (39.43 KB, patch)
2014-08-30 16:42 PDT, Brian Burg 		timothy: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Brian Burg
Comment 1 2014-08-30 16:42:34 PDT
Created attachment 237418 [details] Patch
Timothy Hatcher
Comment 2 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.
Joseph Pecoraro
Comment 3 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?
Joseph Pecoraro
Comment 4 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?
Brian Burg
Comment 5 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?
Timothy Hatcher
Comment 6 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...
Timothy Hatcher
Comment 7 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.
Brian Burg
Comment 8 2014-09-02 21:59:05 PDT
Committed r173199: <http://trac.webkit.org/changeset/173199>
Note You need to log in before you can comment on or make changes to this bug.