Paul Bone [:pbone] Assignee
	Description • 6 years ago

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.

	Paul Bone [:pbone] Assignee
	Comment 1 • 6 years ago

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.

	Jan de Mooij [:jandem]
	Comment 2 • 6 years ago

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

	Jan de Mooij [:jandem]
	Comment 3 • 6 years ago

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.

	Jan de Mooij [:jandem]
	Comment 4 • 6 years ago

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

	Kannan Vijayan [:djvj]
	Comment 5 • 6 years ago

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.

	Paul Bone [:pbone] Assignee
	Comment 6 • 6 years ago

(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.

	Paul Bone [:pbone] Assignee
	Comment 7 • 6 years ago

So jandem, djvj.

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

	Paul Bone [:pbone] Assignee
	Comment 8 • 6 years ago

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

Re-requesting review.

	Jan de Mooij [:jandem]
	Comment 9 • 6 years ago

(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!

	Jan de Mooij [:jandem]
	Comment 10 • 6 years ago

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

	Paul Bone [:pbone] Assignee
	Comment 11 • 6 years ago

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

	Paul Bone [:pbone] Assignee
	Comment 12 • 6 years ago

(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.

	Jan de Mooij [:jandem]
	Comment 13 • 6 years ago

(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

	Paul Bone [:pbone] Assignee
	Comment 14 • 6 years ago

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.

	Paul Bone [:pbone] Assignee
	Comment 16 • 6 years ago

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

	Markus Stange [:mstange]
	Comment 17 • 6 years ago

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.

	Paul Bone [:pbone] Assignee
	Comment 18 • 6 years ago

Thanks.

	Jan de Mooij [:jandem]
	Comment 19 • 6 years ago

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();
}

	Pulsebot
	Comment 20 • 6 years ago

Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d525f6dee9f
Discard JIT stubs when discarding other code r=jandem

	Dorel Luca [:dluca]
	Comment 21 • 6 years ago
bugherder

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