Bug 19229 - JSProfiler: Find more descriptive information to display than (Anonymous Function)
Summary: JSProfiler: Find more descriptive information to display than (Anonymous Func...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Kevin McCullough
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-05-23 17:20 PDT by Kevin McCullough
Modified: 2013-10-04 10:18 PDT (History)
8 users (show)

See Also:


Attachments
Test case (2.86 KB, application/zip)
2008-09-25 15:21 PDT, Kevin McCullough
no flags Details
patch (19.38 KB, patch)
2009-06-24 05:03 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
patch with unlikely (19.14 KB, patch)
2009-06-26 03:42 PDT, Alexandru Chiculita
achicu: review-
Details | Formatted Diff | Diff
patch fix (20.47 KB, patch)
2009-06-26 05:35 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin McCullough 2008-05-23 17:20:08 PDT
If a called function is a FunctionImp but doesn't have a function name, it is displayed in the profiler as "(Anonymous Function)" but this is true for prototypes and many sites use prototypes for the majority of their development.  It would be better if we had a way to display more useful information.

<rdar://problem/5958825> JSProfiler: Find more descriptive information to display than (Anonymous Function)
Comment 1 Kevin McCullough 2008-09-25 15:21:36 PDT
Created attachment 23824 [details]
Test case
Comment 2 Oliver Hunt 2008-09-26 05:14:35 PDT
Kevin and I spent some time discussing this today :D
Comment 3 Alexandru Chiculita 2009-06-24 05:03:13 PDT
Created attachment 31781 [details]
patch

Made AssignDotNode, PropertyNode, AssignResolveNode propagate the identifier to the child node if it is a FuncExprNode. The name is saved on the FuncBodyNode. The JSFunction::calculatedDisplayName will try to use that one only if InternalFunction::calculatedDisplayName() returns an empty String.
Comment 4 Oliver Hunt 2009-06-24 05:10:19 PDT
Comment on attachment 31781 [details]
patch

Have you run performance numbers on this patch? (adding a virtual call to every normal assignment scares me a little) and i am concerned about the increase in size to FunctionBodyNode, although i don't have a really good solution that would avoid it.  In general the patch looks fine but i'm too tired to do a proper review.
Comment 5 Alexandru Chiculita 2009-06-24 06:32:36 PDT
(In reply to comment #4)
> (From update of attachment 31781 [details] [review])
> Have you run performance numbers on this patch? (adding a virtual call to every
> normal assignment scares me a little) and i am concerned about the increase in
> size to FunctionBodyNode, although i don't have a really good solution that
> would avoid it.  In general the patch looks fine but i'm too tired to do a
> proper review.
> 
It should affect the parse only and I don't know if SunSpider catches the parser speed. I've pasted the Sunspider results on http://pastebin.com/m76a3ee44 ("From" is CL @45069 build with release and "To" is @45069 with this patch)
Comment 6 Alexandru Chiculita 2009-06-26 00:38:38 PDT
Results of WebKitTools/Scripts/run-sunspider on macosx: http://pastebin.com/m21b4f477
Comment 7 Oliver Hunt 2009-06-26 00:48:25 PDT
Comment on attachment 31781 [details]
patch

clearing review as i think i misread the perf numbers
Comment 8 Oliver Hunt 2009-06-26 00:51:30 PDT
A 1% regression isn't really something we can land :-/
Comment 9 Alexandru Chiculita 2009-06-26 03:42:33 PDT
Created attachment 31925 [details]
patch with unlikely

Added UNLIKELY to ifs. It seems to improve speed.

("From" is the unchanged code): http://pastebin.com/m3a17662d
Comment 10 Alexandru Chiculita 2009-06-26 04:07:36 PDT
Comment on attachment 31925 [details]
patch with unlikely

It doesn't propagate the name to the new body when the functions are recompiled.
Comment 11 Alexandru Chiculita 2009-06-26 05:35:59 PDT
Created attachment 31929 [details]
patch fix

- added setContextualName calls in JavaScriptDebugServer.cpp when it recompiles all the js functions;
- changed getContextualName() to contextualName()
Comment 12 Maciej Stachowiak 2009-07-05 04:14:28 PDT
Are there fresh performance numbers on this? Does it still cause a 1% regression?
Comment 13 Alexandru Chiculita 2009-07-05 05:51:27 PDT
(In reply to comment #12)
> Are there fresh performance numbers on this? Does it still cause a 1%
> regression?

"patch fix" performance result are at http://pastebin.com/m3a17662d.

I've just added UNLIKELY in the last patch and it seems faster. I don't know how it works on Windows. 

At the moment when those script nodes are created the profiler might not be enabled yet, so this change cannot be activated only when the profiler is enabled.  Is there any way to determine if the "Show develop menu in menubar" option is checked, so that the code could bypass the useless virtual call to isFuncExprNode?.
Comment 14 Oliver Hunt 2009-07-15 23:59:54 PDT
Comment on attachment 31929 [details]
patch fix

Have you actually tested performance? You can simply use the run-sunspider script on the commandline to test this; "run-sunspider --runs 30" should give you a stable number.  "run-sunspider --runs 30 > baseTimes", etc for before and after numbers then "sunspider-compare-results baseTimes newTimes" will give you actual performance comparison
Comment 15 Timothy Hatcher 2009-08-07 09:23:10 PDT
Comment on attachment 31929 [details]
patch fix

Please put this up for review again one performance has been tested.
Comment 16 Timothy Hatcher 2013-10-04 10:18:09 PDT
We did this a while ago in another patch.