Closed Bug 1776721 Opened 2 years ago Closed 2 years ago

Change the API to add label frames in the JS code

Categories

(Core :: Gecko Profiler, task, P3)

Product:
Core
Component:
Gecko Profiler
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
104 Branch
Tracking Flags:
Tracking Status
firefox104 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(5 files)

Bug 1776721 - Do not push a profiler frame when it's not running r=jandem
48 bytes, text/x-phabricator-request
Details | Review
Bug 1776721 - Add a new constructor to AutoGeckoProfilerEntry to make it possible to push a label frame with a dynamic string r=jandem
48 bytes, text/x-phabricator-request
Details | Review
Bug 1776721 - Provide new RAII classes to make it easier specifying RELEVANT_FOR_JS label frames in a performant way r=jandem
48 bytes, text/x-phabricator-request
Details | Review
Bug 1776721 - Improve a few comments around the category generating script r=canaltinova
48 bytes, text/x-phabricator-request
Details | Review
Bug 1776721 - Add a new sub-category for javascript builtins r=canaltinova,jandem
48 bytes, text/x-phabricator-request
Details | Review

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

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.

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

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.

Severity: -- → N/A
Priority: -- → P3

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/

Pushed by jwajsberg@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: