1749521 - We should have "relevant for JS" label frames for JavaScript static API methods

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P2)

Description

[Julien Wajsberg [:julienw]]

Attachment: object.html

From the attachment, I generated this profile:
https://share.firefox.dev/3qfp4q7

We clearly see that obj_keys is present, which is the implementation for Object.keys.

Now if we switch to "JavaScript" stacks we don't see it anymore.

I believe there should be a label frame for this function as well as similar ones.

See https://searchfox.org/mozilla-central/rev/7271a078fa0c1b858a52614ea60ac82fdd8b3d23/js/src/builtin/Object.cpp#2166-2191 for other functions that probably share the same issue (but I haven't double checked for those). I double checked that instance methods have the same issue though (eg: Object.prototype.propertyIsEnumerable).

I also double-checked this for the methods in Array: they share the same issue, for example Array.of has the same issue. However methods on the prototype have a correct label frame.

Therefore it would be good to have a fix that span all builtins implemented with JS_FN.

Comment 1

[Julien Wajsberg [:julienw]]

We looked at it with Nazim, and we've noticed that the methods of Array that "works" have manually specified a label frame (eg [1]) but it looks like that it's not done for all of them.

We were wondering if we could wrap the call function pointer in [2] in a lambda for example.

Last solution would be to manually specify all label frames in all the proper locations. That's tedious and not future proof, but it may be the only practical solution.

Hey jandem, by chance, would you have any insight about this?

[1] https://searchfox.org/mozilla-central/rev/83e67336083df9f9a3d1e0c33f2ba19703d57161/js/src/builtin/Array.cpp#1277-1279
[2] https://searchfox.org/mozilla-central/rev/83e67336083df9f9a3d1e0c33f2ba19703d57161/js/public/PropertySpec.h#444-445

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Julien Wajsberg [:julienw] from comment #1)

We were wondering if we could wrap the call function pointer in [2] in a lambda for example.

Last solution would be to manually specify all label frames in all the proper locations. That's tedious and not future proof, but it may be the only practical solution.

I think both of these would have too much overhead. Performance overhead, but it would also bloat the binary and source code because there are a lot of built-ins.

We don't have a great solution for this atm. Maybe a generic "non-JS" frame in the profiler's JS view that we use for all C++ frames?

Comment 3

[Markus Stange [:mstange]]

For WebIDL calls I've invested significant effort into making its generated code as small as possible, see bug 1499507 comment 1 for some history. If it's good enough for WebIDL (>10000 calls), I'd hope it would be good enough for Spidermonkey too.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Markus Stange [:mstange] from comment #3)

For WebIDL calls I've invested significant effort into making its generated code as small as possible, see bug 1499507 comment 1 for some history. If it's good enough for WebIDL (>10000 calls), I'd hope it would be good enough for Spidermonkey too.

Oh right, I forgot we already do this for WebIDL natives. For SpiderMonkey we'd have to add this manually to all natives though which is a bit of a pain (we have some vague ideas around using a WebIDL-like scheme to generate code but that's not happening anytime soon).

Comment 6

[Julien Wajsberg [:julienw]]

To reduce the bloat, we could pick only the ones that are more likely to bring performance problems, such as loops like Object.keys.

Comment 7

[Julien Wajsberg [:julienw]]

I went with the approach of adding directly to the c++ methods.

Comment 8

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to most methods of the Object object r=jandem

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some methods of the TypedArray object r=jandem

Depends on D150133

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some methods of numbers r=jandem

Depends on D150134

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some methods of the Date object r=jandem

Depends on D150135

Comment 12

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to constructors of the Intl objects r=jandem

Depends on D150136

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some methods of RegExp objects r=jandem

Depends on D150137

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some methods and functions of String r=jandem

Depends on D150138

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some static methods of Reflect r=jandem

Depends on D150139

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some methods of Map and Set r=jandem

Depends on D150140

Comment 17

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to the static methods of the JSON object r=jandem

Depends on D150141

Comment 18

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to some methods of BigInt r=jandem

Depends on D150142

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: Bug 1749521 - Add label frames relevant for JavaScript developers to more methods of Array r=jandem

Depends on D150143

Comment 20

[Markus Stange [:mstange]]

Nice work.

To reduce the size taken up by strings, I recommend making use of ProfilingStackFrame::Flags::STRING_TEMPLATE_METHOD and its friends, like we do for WebIDL:

https://searchfox.org/mozilla-central/rev/6d0ba065e3d41822337c708c8c0aca334ddd9218/dom/bindings/Codegen.py#10724-10727

AUTO_PROFILER_LABEL_DYNAMIC_FAST(
  "${interface_name}", "${method_name}", DOM, cx,
  uint32_t(js::ProfilingStackFrame::Flags::STRING_TEMPLATE_METHOD) |
  uint32_t(js::ProfilingStackFrame::Flags::RELEVANT_FOR_JS));

See this commit https://hg.mozilla.org/mozilla-central/rev/5ec157cbbdd11c216e996e2cc763bd087620cb11 :

This means that our binary does not need to include concatenated strings such
as "set CanvasRenderingContext2D.fillStyle". It only needs to contain the
individual strings "CanvasRenderingContext2D" and "fillStyle" which are most
likely already present in the binary.

Comment 21

[Julien Wajsberg [:julienw]]

Try: https://treeherder.mozilla.org/jobs?repo=try&revision=c448390f63b03fa4688586c256281013fc1b42f2
Example profile: https://share.firefox.dev/3R4Noqe (captured with the Web Developer preset, that is without native stacks, and inverted). We can clearly see some new constructors as well as new instumented methods.

Comment 22

[Jan de Mooij [:jandem]]

Could you share some perf numbers for a micro-benchmark? Call one of the faster builtins (Date.now() or date.toSource() maybe?) N times in a loop in a JS function and measure how long this takes before/after these changes. (Either JS shell or an HTML file would work, avoid devtools because it has known perf issues.)

Comment 23

[Jan de Mooij [:jandem]]

"".toLowerCase() is also a good one to check.

Comment 24

[Julien Wajsberg [:julienw]]

Attachment: javascript-micro-benchmark.html

With this HTML file, I checked Date.now(), "".toLowerCase(), as well as [].includes() which had a label frame already, to measure impact of bug 1776721 as well.

The results are:

[].includes(): 17% improvement
Date.now(): 1% regression
"".toLowerCase(): 1% regression

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fe38b091446
Add label frames relevant for JavaScript developers to most methods of the Object object r=jandem
https://hg.mozilla.org/integration/autoland/rev/3fa44d29b206
Add label frames relevant for JavaScript developers to some methods of the TypedArray object r=jandem
https://hg.mozilla.org/integration/autoland/rev/59206e90e55a
Add label frames relevant for JavaScript developers to some methods of numbers r=jandem
https://hg.mozilla.org/integration/autoland/rev/bb4018ade890
Add label frames relevant for JavaScript developers to some methods of the Date object r=jandem
https://hg.mozilla.org/integration/autoland/rev/057335805cd9
Add label frames relevant for JavaScript developers to constructors of the Intl objects r=jandem
https://hg.mozilla.org/integration/autoland/rev/6e9d12f5a336
Add label frames relevant for JavaScript developers to some methods of RegExp objects r=jandem
https://hg.mozilla.org/integration/autoland/rev/dbe28d550837
Add label frames relevant for JavaScript developers to some methods and functions of String r=jandem
https://hg.mozilla.org/integration/autoland/rev/b210832c5ada
Add label frames relevant for JavaScript developers to some static methods of Reflect r=jandem
https://hg.mozilla.org/integration/autoland/rev/8c577e3df203
Add label frames relevant for JavaScript developers to some methods of Map and Set r=jandem
https://hg.mozilla.org/integration/autoland/rev/f3500cbcfe2c
Add label frames relevant for JavaScript developers to the static methods of the JSON object r=jandem
https://hg.mozilla.org/integration/autoland/rev/40b0333164e2
Add label frames relevant for JavaScript developers to some methods of BigInt r=jandem
https://hg.mozilla.org/integration/autoland/rev/a81add5fb3b0
Add label frames relevant for JavaScript developers to more methods of Array r=jandem

Comment 26

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/2fe38b091446
https://hg.mozilla.org/mozilla-central/rev/3fa44d29b206
https://hg.mozilla.org/mozilla-central/rev/59206e90e55a
https://hg.mozilla.org/mozilla-central/rev/bb4018ade890
https://hg.mozilla.org/mozilla-central/rev/057335805cd9
https://hg.mozilla.org/mozilla-central/rev/6e9d12f5a336
https://hg.mozilla.org/mozilla-central/rev/dbe28d550837
https://hg.mozilla.org/mozilla-central/rev/b210832c5ada
https://hg.mozilla.org/mozilla-central/rev/8c577e3df203
https://hg.mozilla.org/mozilla-central/rev/f3500cbcfe2c
https://hg.mozilla.org/mozilla-central/rev/40b0333164e2
https://hg.mozilla.org/mozilla-central/rev/a81add5fb3b0