Closed Bug 1494278 Opened Last year Closed Last year

JITed allocation counters for perf.html are broken

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
thunderbird_esr52 --- unaffected
thunderbird_esr60 --- unaffected
geckoview62 --- unaffected
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Attachment #9012487 - Flags: review?(kvijayan)
Does this mean the allocation counts are now always enabled? I thought we explicitly wanted to avoid that.
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.
Do you know where these allocations are coming from? If it's trampoline code we could discard them when toggling profiler state.
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.
(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.
Blocks: 1490917
So jandem, djvj.

Do you think it's okay that this is used when the profiler is installed?  Is it okay to land?
Flags: needinfo?(kvijayan)
Flags: needinfo?(jdemooij)
Comment on attachment 9012487 [details] [diff] [review]
Bug 1494278 - Enable JITed allocation counters if the profiler is installed

Re-requesting review.
Attachment #9012487 - Flags: review?(kvijayan)
(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!
Flags: needinfo?(jdemooij)
Oh and could the missing objects be from trampoline code, maybe? If it involves regular expressions or JSON that could suggest it.
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
Flags: needinfo?(mstange)
(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.
(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
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.
Flags: needinfo?(mstange)
Attachment #9012487 - Attachment is obsolete: true
Attachment #9012487 - Flags: review?(kvijayan)
Attachment #9014384 - Flags: review?(jdemooij)
Whoops, clearned the wrong needinfo, I still want to know what installed() means for the profiler and maybe comment it.
Flags: needinfo?(kvijayan) → needinfo?(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.
Flags: needinfo?(mstange)
Thanks.
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();
}
Attachment #9014384 - Flags: review?(jdemooij) → review+
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d525f6dee9f
Discard JIT stubs when discarding other code r=jandem
https://hg.mozilla.org/mozilla-central/rev/6d525f6dee9f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.