1776721 - Change the API to add label frames in the JS code

Status: RESOLVED FIXED

(Core :: Gecko Profiler, task, P3)

Description

[Julien Wajsberg [:julienw]]

The current API to add label frames [1] has several shortcomings:

• it always adds the frames even when the profiler isn't running. This is something desirable for long-running tasks so that we can see the frames when starting the profiler after the long-running task started, but this is statistically useless for short-running tasks and takes time.
• it forces the caller to provide the full name of each label (see [2]), which duplicates a lot of strings

In this bug I'll address these first problem by mimicking the "FAST" flavors of macros (see [3]), and I'll address the second problem by using the flag STRING_TEMPLATE_METHOD. These 2 changes were implemented to make the performance acceptable so that we could use it for all of WebIDL, and therefore I expect this will be acceptable enough to add label frames more broadly in the JS API code (bug 1749521).

[1] https://searchfox.org/mozilla-central/rev/bf6f194694c9d1ae92847f3d4e4c15c2486f3200/js/src/vm/GeckoProfiler-inl.h#83-114
[2] https://searchfox.org/mozilla-central/rev/bf6f194694c9d1ae92847f3d4e4c15c2486f3200/js/src/builtin/Array.cpp#3641-3643
[3] https://searchfox.org/mozilla-central/rev/bf6f194694c9d1ae92847f3d4e4c15c2486f3200/tools/profiler/public/ProfilerLabels.h#167-175

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: Bug 1776721 - Do not push a profiler frame when it's not running r=jandem

The assumption for this patch is that the places where we're adding
label frames are short-lived, and therefore it's OK if we miss these
label frames at the start of a profile.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: Bug 1776721 - Add a new constructor to AutoGeckoProfilerEntry to make it possible to push a label frame with a dynamic string r=jandem

Depends on D150407

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: Bug 1776721 - Provide new RAII classes to make it easier specifying RELEVANT_FOR_JS label frames in a performant way r=jandem

Two classes are introduced: AutoJsMethodProfilerEntry and
AutoJsConstructorProfilerEntry. AutoJsMethodProfilerEntry makes use of
the flag STRING_TEMPLATE_METHOD to concatenate the object and method
names, and both classes use the flag RELEVANT_FOR_JS.

Depends on D150408

Comment 4

[Julien Wajsberg [:julienw]]

Here is a profile captured with this patch: https://share.firefox.dev/3OGy2pK
(in this case with array.prototype already searched for, and inverted call tree so that it's more visible). Normally there's no difference with what happens on central.
In the same profile https://share.firefox.dev/3A9CqcS you can also look for script emit or performSweepActions for other examples that aren't exposed to JS developers.

Comment 5

[Julien Wajsberg [:julienw]]

The try from bug 1749521 contains also these patches: https://treeherder.mozilla.org/jobs?repo=try&revision=c448390f63b03fa4688586c256281013fc1b42f2

Comment 6

[Julien Wajsberg [:julienw]]

The test Spidermonkey builds spidermonkey-sm-nonunified-linux64/debug nu gives me:

[task 2022-06-28T15:28:58.910Z] /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -std=gnu++17 -o GeckoProfiler.o -c  -I/builds/worker/workspace/obj-spider/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -ftrivial-auto-var-init=pattern -DDEBUG=1 -DWASM_SUPPORTS_HUGE_MEMORY -DJS_CACHEIR_SPEW -DJS_STRUCTURED_SPEW -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/builds/worker/checkouts/gecko/js/src -I/builds/worker/workspace/obj-spider/js/src -I/builds/worker/workspace/obj-spider/dist/include -I/builds/worker/workspace/obj-spider/dist/include/nspr -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-spider/js/src/js-confdefs.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wcomma -Wenum-compare-conditional -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=free-nonheap-object -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wthread-safety -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-spider/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -Werror=format -fstandalone-debug -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/GeckoProfiler.o.pp   /builds/worker/checkouts/gecko/js/src/vm/GeckoProfiler.cpp
[task 2022-06-28T15:28:58.911Z] In file included from /builds/worker/checkouts/gecko/js/src/vm/GeckoProfiler.cpp:7:
[task 2022-06-28T15:28:58.911Z] In file included from /builds/worker/checkouts/gecko/js/src/vm/GeckoProfiler-inl.h:10:
[task 2022-06-28T15:28:58.912Z] /builds/worker/checkouts/gecko/js/src/vm/GeckoProfiler.h:200:3: error: unknown type name 'ProfilingStack'
[task 2022-06-28T15:28:58.912Z]   ProfilingStack* profilingStack_;
[task 2022-06-28T15:28:58.912Z]   ^
[task 2022-06-28T15:28:58.912Z] /builds/worker/checkouts/gecko/js/src/vm/GeckoProfiler.h:200:19: error: private field 'profilingStack_' is not used [-Werror,-Wunused-private-field]
[task 2022-06-28T15:28:58.912Z]   ProfilingStack* profilingStack_;
[task 2022-06-28T15:28:58.912Z]                   ^
[task 2022-06-28T15:28:58.912Z] 2 errors generated.
[task 2022-06-28T15:28:58.912Z] gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:669: GeckoProfiler.o] Error 1

Not sure what's happening I found a solution \o/

Comment 7

[Julien Wajsberg [:julienw]]

This is a try with the fix for the "nu" test: https://treeherder.mozilla.org/jobs?repo=try&revision=638aee72ab763475e4a544af296d602e14cb5eb1

Comment 8

[Julien Wajsberg [:julienw]]

Attachment: Bug 1776721 - Improve a few comments around the category generating script r=canaltinova

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: Bug 1776721 - Add a new sub-category for javascript builtins r=canaltinova,jandem

Depends on D150613

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35d6004029ee
Improve a few comments around the category generating script r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/75ef0816f1a9
Add a new sub-category for javascript builtins r=canaltinova,jandem
https://hg.mozilla.org/integration/autoland/rev/529171f9ae9f
Do not push a profiler frame when it's not running r=jandem
https://hg.mozilla.org/integration/autoland/rev/ea47ad1d2a1d
Add a new constructor to AutoGeckoProfilerEntry to make it possible to push a label frame with a dynamic string r=jandem
https://hg.mozilla.org/integration/autoland/rev/7ccf9715e81d
Provide new RAII classes to make it easier specifying RELEVANT_FOR_JS label frames in a performant way r=jandem

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/35d6004029ee
https://hg.mozilla.org/mozilla-central/rev/75ef0816f1a9
https://hg.mozilla.org/mozilla-central/rev/529171f9ae9f
https://hg.mozilla.org/mozilla-central/rev/ea47ad1d2a1d
https://hg.mozilla.org/mozilla-central/rev/7ccf9715e81d