1393597 - NotifyOffThreadScriptLoadCompletedRunnable hangs a lot

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

In BHR we have some idea of which runnables are causing the most hangs. NotifyOffThreadScriptLoadCompletedRunnable is responsible for a very high number of >2s hangs in the tab process.

(not sure how stable this URL is but): https://squarewave.github.io/?durationSpec=2048_65536&onlyUserInteracting=true&runnable=dom%3A%3ANotifyOffThreadScriptLoadCompletedRunnable%28labeled%29&search=FinishIncrementalGC&thread=0

This is caused in a decent number of cases because we call FinishIncrementalGC in this runnable, which can be quite expensive. 

It would be nice to avoid that.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Ehsan suggests you might have some familiarity with this :-).

Comment 2

[Jan de Mooij [:jandem]]

GlobalHelperThreadState::mergeParseTaskCompartment will FinishIncrementalGC if we're doing an incremental GC that affects the zone that triggered the off-thread compilation. According to the profile that's what's happening here.

Bug 1371908 improved the situation a bit but it's still a potential perf cliff.

Jon, any thoughts? Do you know what would be needed to remove that FinishIncrementalGC completely? We could mark all off-thread allocated GC things I guess?

Comment 3

[Jan de Mooij [:jandem]]

(I'd be happy to try potential fixes for this if you think it's possible to get rid of this call.)

Comment 4

[Jon Coppeard (:jonco)]

Good idea.  I'll give it a try.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1393597-remove-merge-gc

So this is slightly terrifying but it seems to work.

We use reuse GCRuntime::arenaAllocatedDuringGC() to handle adding new arenas to a zone while an incremental GC is in progress.  This ensures the contents get marked if we're marking and do not treated as dead if we're sweeping.

I had to work around the fact that the collector expects the cursor to be at the end of each arena list during sweeping but I don't think this is too big a deal.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=576479e49484d8678fac92a29b7de33db93a6970&selectedJob=126736108

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8902301 [details] [diff] [review]
bug1393597-remove-merge-gc

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

Somewhat terrifying, yes, but in a way it's easier to understand when the minimum necessary is being done instead of sweeping things under a "just avoid this situation because it's problematic for reasons we don't want to deal with" rug.

::: js/src/jsgc.cpp
@@ +7794,5 @@
>              arena->zone = target->zone();
> +            if (MOZ_UNLIKELY(target->zone()->wasGCStarted())) {
> +                arena->unmarkAll();
> +                if (!arena->isEmpty())
> +                    arenaAllocatedDuringGC(target->zone(), arena);

Could this be commented to say that you're marking it allocated during GC to prevent anything from getting discarded during the current GC, or something? I know that's the whole point of allocated-during-GC, but it's actually not necessarily true that all of these arenas were allocated during the GC, unless you think of it as the rezoned arenas being allocated during GC (right here, during the retargeting).

Anyway, you're sort of reusing this flag for a slightly different purpose, which should be commented at least minimally.

@@ +7989,5 @@
>      }
>  }
>  
>  void
> +ArenaLists::adoptArenas(JSRuntime* rt, ArenaLists* fromArenaLists, bool zoneIsCollecting)

Please use the full name targetZoneIsCollecting.

@@ +8013,5 @@
> +            // If the target zone is being swept (either foreground or
> +            // background sweeping) then we need to add the arenas because the
> +            // collector which assumes that the cursor is always at the end of
> +            // the list. This prevents allocation into any non-full arenas until
> +            // the end of the next GC.

I *think* you meant

            // If the target zone is being swept (either foreground or
            // background sweeping) then we need to add the arenas
            // before the cursor because the
            // collector assumes that the cursor is always at the end of
            // the list. This prevents allocation into any non-full arenas until
            // the end of the next GC.

? And if I am understanding the last sentence correctly, I wonder if it could be

  // Even if the adopted arenas are non-full, they will not be used for allocation until the end of the next GC.

I don't think it's different, but it was confusing me because I read it as somehow implying that you could move the cursor around the list to control whether you allocate into non-full arenas. Or something. Which is sort of true, but it's not the main point.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] (PTO Aug 16-23) from comment #6)

> Could this be commented to say that you're marking it allocated during GC to
> prevent anything from getting discarded during the current GC, or something?

Sure.

> I know that's the whole point of allocated-during-GC, but it's actually not
> necessarily true that all of these arenas were allocated during the GC,
> unless you think of it as the rezoned arenas being allocated during GC
> (right here, during the retargeting).

That's what I was intending - this operation is equivalent to allocating all these things in the new zone.

> I *think* you meant

Sorry, I had fixed the mangled comment but uploaded the old patch.  Yes, that's what I meant.  I'll improve the comment to make it clear that the behaviour of non-full arenas is a side effect and not the goal.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acd4c55de436
Remove FinishIncrementalGC when merging compartments r=sfink

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/acd4c55de436

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Is it possible that this has caused some Speedometer regression we're seeing right now?
If we don't run GC early enough, we may run it while running the test - just speculating here.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

The regression bug is bug 1395949

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(There is bug 1377131 which is trying to figure out better time to GC/CC when running artificial benchmarks.)

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Olli Pettay [:smaug] from comment #10)
It's possible.  Not sure what we can do about it though - it feels like this change should be a net improvement.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

yeah, I agree. For non-realistic benchmarking setups, like Speedometer, we may need to have something similar to bug 1377131.