NotifyOffThreadScriptLoadCompletedRunnable hangs a lot

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: jonco)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [bhr])

Attachments

(1 attachment)

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

Comment 1

2 years ago
Ehsan suggests you might have some familiarity with this :-).
Flags: needinfo?(jdemooij)
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?
Flags: needinfo?(jdemooij) → needinfo?(jcoppeard)
(I'd be happy to try potential fixes for this if you think it's possible to get rid of this call.)
(Assignee)

Comment 4

2 years ago
Good idea.  I'll give it a try.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 5

2 years ago
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
Assignee: nobody → jcoppeard
Attachment #8902301 - Flags: review?(sphink)
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.
Attachment #8902301 - Flags: review?(sphink) → review+
(Assignee)

Comment 7

2 years ago
(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

2 years ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acd4c55de436
Remove FinishIncrementalGC when merging compartments r=sfink

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/acd4c55de436
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
The regression bug is bug 1395949
(There is bug 1377131 which is trying to figure out better time to GC/CC when running artificial benchmarks.)
(Assignee)

Comment 13

2 years ago
(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.
yeah, I agree. For non-realistic benchmarking setups, like Speedometer, we may need to have something similar to bug 1377131.
You need to log in before you can comment on or make changes to this bug.