1494278 - JITed allocation counters for perf.html are broken

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Paul Bone [:pbone]]

The nursery allocation counter isn't correctly enabled when the profiler is enabled.  This is introduced in Bug 1480001.

I noticed weird allocation counts when investigating Bug 1490917 and found that if I disabled JIT or uncommented the profiler checks around these nursery counts then I got correct information.

I suspect that we're not discarding JITed code with the profiler is enabled, which I thought was the case.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1494278 - Enable JITed allocation counters if the profiler is installed

Previously we enabled them when the profiler is enabled, but baseline JITed
code is never thrown away when the profiler starts.  And so not all JITed
code used the counters and they were inaccurate.

Comment 2

[Jan de Mooij [:jandem]]

Does this mean the allocation counts are now always enabled? I thought we explicitly wanted to avoid that.

Comment 3

[Jan de Mooij [:jandem]]

When we enable the profiler, we should discard almost all JIT code, including Baseline code:

https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/js/src/vm/GeckoProfiler.cpp#93-97

The only exception is Baseline code that's still on the stack, but I don't know if that's common when starting the profiler.

Comment 4

[Jan de Mooij [:jandem]]

Do you know where these allocations are coming from? If it's trampoline code we could discard them when toggling profiler state.

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 9012487 [details] [diff] [review]
Bug 1494278 - Enable JITed allocation counters if the profiler is installed

Review of attachment 9012487 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks fine, but I'd like to echo :jandem's question: do we really want to collect this when profiling is not enabled?  Maybe we do.. but I'd like to hear the reasoning.

Comment 6

[Paul Bone [:pbone]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Does this mean the allocation counts are now always enabled? I thought we
> explicitly wanted to avoid that.

I think that geckoProfiler().installed() is true only when the profiler addon is installed.  So this will only impact people who have the profiler.  Anyone not interested in Firefox or maybe web development will not be affected.

(In reply to Jan de Mooij [:jandem] from comment #3)
> When we enable the profiler, we should discard almost all JIT code,
> including Baseline code:
> 
> https://searchfox.org/mozilla-central/rev/
> ce57be88b8aa2ad03ace1b9684cd6c361be5109f/js/src/vm/GeckoProfiler.cpp#93-97
> 
> The only exception is Baseline code that's still on the stack, but I don't
> know if that's common when starting the profiler.

I found a case of some code in gmail (Bug 1490917) where the counts were very low, missing a huge active loop that was allocating thousands of objects.

I don't know exactly how the loop starts, but my guess is that it was baseline code with active frames.

I also think that one way people use the profiler is they start it either just before performing an action that creates jank, or soon after the jank begins, so we could be missing out on this baseline code in this way.

Comment 7

[Paul Bone [:pbone]]

So jandem, djvj.

Do you think it's okay that this is used when the profiler is installed?  Is it okay to land?

Comment 8

[Paul Bone [:pbone]]

Comment on attachment 9012487 [details] [diff] [review]
Bug 1494278 - Enable JITed allocation counters if the profiler is installed

Re-requesting review.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Paul Bone [:pbone] from comment #6)
> I think that geckoProfiler().installed() is true only when the profiler
> addon is installed.  So this will only impact people who have the profiler. 
> Anyone not interested in Firefox or maybe web development will not be
> affected.

When I quickly looked into this last week, I thought the profiler stack is always "installed" for the main thread JSContext? I think "installed" here is not the same thing as "the addon is installed" but "we installed/prepared the profiler data structures". Correct me if I'm wrong!

Comment 10

[Jan de Mooij [:jandem]]

Oh and could the missing objects be from trampoline code, maybe? If it involves regular expressions or JSON that could suggest it.

Comment 11

[Paul Bone [:pbone]]

Markus do you know what installed() means for the gecko profiler thread C++ class?  Can I use it to determine whether the user has the add-on installed?  Are there other ways to enable instrumentation?

https://searchfox.org/mozilla-central/source/js/public/ProfilingStack.h#482

Comment 12

[Paul Bone [:pbone]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> Oh and could the missing objects be from trampoline code, maybe? If it
> involves regular expressions or JSON that could suggest it.

It definitly involves regular expressions.  It includes a bunch of objects such as regular expression match results.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Paul Bone [:pbone] from comment #12)
> It definitly involves regular expressions.  It includes a bunch of objects
> such as regular expression match results.

In that case the proper fix is probably to clear (set to nullptr) these stubs in ReleaseAllJITCode:

https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/js/src/jit/JitRealm.h#507

Comment 14

[Paul Bone [:pbone]]

Thanks jandem,

That works, the only gripe is that it's slow to react, there's a delay after the profiler starts but before the new code is running and records the counts correctly.  AFAIK that's unavoidable because this is code that's already running when the profiler starts.

Comment 15

[Paul Bone [:pbone]]

Attachment: Bug 1494278 - Discard JIT stubs when discarding other code

Comment 16

[Paul Bone [:pbone]]

Whoops, clearned the wrong needinfo, I still want to know what installed() means for the profiler and maybe comment it.

Comment 17

[Markus Stange [:mstange]]

I think installed() is always true (in builds which include profiler code, which is all builds on our Tier 1 platforms), except very early during startup or very late during shutdown. It basically reflects whether outside-of-Spidermonkey code has had a chance to register the gecko profiler with inside-of-Spidermonkey code.
GeckoProfilerRuntime::enabled() (cx->runtime()->geckoProfiler().enabled()) returns whether the profiler is currently running.
There is no way to check whether the Gecko Profiler add-on is installed.

Comment 18

[Paul Bone [:pbone]]

Thanks.

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 9014384 [details] [diff] [review]
Bug 1494278 - Discard JIT stubs when discarding other code

Review of attachment 9014384 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/gc/GC.cpp
@@ +8820,5 @@
>      }
> +
> +    for (RealmsIter realm(fop->runtime()); !realm.done(); realm.next()) {
> +        jit::JitRealm *jitRealm = realm->jitRealm();
> +        if (jitRealm) {

Nit: * goes with the type and while you're here, we often combine it with the if-statement like:

if (jit::JitRealm* jitRealm = realm->jitRealm()) {
    jitRealm->discardStubs();
}

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d525f6dee9f
Discard JIT stubs when discarding other code r=jandem

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6d525f6dee9f