1470916 - No way to distinguish between "bound" functions of the same arity

Status: NEW

(Core :: Gecko Profiler, enhancement, P3)

Description

[Julien Wajsberg [:julienw]]

Attachment: testcase-bound-functions.html

STR:
1. Load the testcase attachment.
2. Start the profiler.
3. Click on the button.
4. Capture the profile
5. Switch to "JS only"

=> Notice how the 2 bound functions are below the same function "bound"

See also the online profile: https://perfht.ml/2Khr61t
And the attachment.


Some other comments:
* a bound function's name is "bound <func name>" (`func.bind(null).name`). It means we don't get the function name here, but rather directly the returned function called "bound" in the JS engine [1].
* The JS engine has 4 different versions of this "bound" function, depending on the number of the arguments to "bind".

[1] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/js/src/builtin/Function.js#62

I think we should have different entries for each different bound function.
One edge case is about different bound functions for the same underlying function: should we get one entry or several entries? Logically it should be several different functions, but it may make it difficult to find errors.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: profiler's screenshot

Comment 2

[Markus Stange [:mstange]]

(In reply to Julien Wajsberg [:julienw] from comment #0)
> It means we don't get the function name here, but rather directly the
> returned function called "bound" in the JS engine [1].

This is what I would expect. If I wrote "return function foo() { ... }" in my own code, then I'd expect invocations of it to show up with the name foo. Are you suggesting that the JS engine should special-case "bound" because it's so common? Or are you saying the function name that's exposed to the profiler should be derived from functionObject.name for all functions?

> * The JS engine has 4 different versions of this "bound" function, depending
> on the number of the arguments to "bind".

Do we really collapse these four different versions into the same func? If we do that, it's a bug. But I'd expect those different versions to have different line numbers; e.g. the zero-argument examples in your testcase show up as self-hosted:958 and I'd expect the other versions to have different line numbers. Those different line numbers should cause perf.html to create separate funcs for them.

Comment 3

[Julien Wajsberg [:julienw]]

> If I wrote "return function foo() { ... }" in my own code, then I'd expect invocations of it to show up with the name foo. Are you suggesting that the JS engine should special-case "bound" because it's so common? Or are you saying the function name that's exposed to the profiler should be derived from functionObject.name for all functions?

I don't know what's the best solution, both would work for me. What I know is that the current situation is a bit confusing...

To add some confusion, when inspecting `new Error().stack` doesn't hold "bound <function name>" but just the initial function name in the stack (see next attachment). Personally I do like seeing the "bound XXX" information to distinguish the bound function from the unbound version.

> Do we really collapse these four different versions into the same func?

no no, we don't :) Sorry for the confusion.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: testcase to display the stack trace in bound functions

Comment 5

[Julien Wajsberg [:julienw]]

> To add some confusion, when inspecting `new Error().stack` doesn't hold "bound <function name>" but just the initial function name in the stack (see next attachment).

Actually, the bound function is erased from the stack. Maybe it's per spec?
Anyway I like it that we see it in profiles, but I don't that all "bound" functions of the same "type" (number of arguments) are merged in inverted trees, as it makes profiles more difficult to analyze.

This may be another case where filtering by JS isn't the right filter, what we want really is filtering by "outside Gecko".

Comment 6

[Greg Tatum [:gregtatum]]

Is there a decision or next step on this bug? It would be nice to update it on what we want to do, or close it as not actionable. For now I'm adding it to the backlog as a P3.

Comment 7

[Julien Wajsberg [:julienw]]

I think we should start by capturing the function's name property if it's set (instead or in addition of what we're doing now), and then decide what to do with it in perf.html. But I don't know if this has a performance penalty.