Bug 19229 – JSProfiler: Find more descriptive information to display than (Anonymous Function)

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kevin McCullough]

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)

[Kevin McCullough]

Created attachment 23824 [details]
Test case

[Oliver Hunt]

Kevin and I spent some time discussing this today :D

[Alexandru Chiculita]

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.

[Oliver Hunt]

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.

[Alexandru Chiculita]

(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)

[Alexandru Chiculita]

Results of WebKitTools/Scripts/run-sunspider on macosx: http://pastebin.com/m21b4f477

[Oliver Hunt]

Comment on attachment 31781 [details]
patch

clearing review as i think i misread the perf numbers

[Oliver Hunt]

A 1% regression isn't really something we can land :-/

[Alexandru Chiculita]

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

[Alexandru Chiculita]

Comment on attachment 31925 [details]
patch with unlikely

It doesn't propagate the name to the new body when the functions are recompiled.

[Alexandru Chiculita]

Created attachment 31929 [details]
patch fix

- added setContextualName calls in JavaScriptDebugServer.cpp when it recompiles all the js functions;
- changed getContextualName() to contextualName()

[Maciej Stachowiak]

Are there fresh performance numbers on this? Does it still cause a 1% regression?

[Alexandru Chiculita]

(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?.

[Oliver Hunt]

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

[Timothy Hatcher]

Comment on attachment 31929 [details]
patch fix

Please put this up for review again one performance has been tested.

[Timothy Hatcher]

We did this a while ago in another patch.