1097302 - Investigate if we could have more major forget skippable runs

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Attachment: v1

Currently the first forget skippable after GC may take quite some time.
I think we should split that to several phases.
(And we should also move some marking to happen during GC, but that is a different bug.)

https://tbpl.mozilla.org/?tree=Try&rev=531fb3909707

Comment 1

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

Comment on attachment 8520963 [details] [diff] [review]
v1

This should help a bit with max forget skippable times.

IMO this makes the code also easier to understand.

Comment 2

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

Comment on attachment 8520963 [details] [diff] [review]
v1

Actually, I want to add yet another phase.

Comment 3

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

Attachment: v2

https://tbpl.mozilla.org/?tree=Try&rev=958854978d08

So yet another phase so that we don't do too much right after GC has run.
But in order to ensure fs runs before CC, dropped the 400ms timer to 250ms.

Comment 4

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

Hmm, no, not good. We get sync forgetSkippables too often.

Comment 5

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

Attachment: v3

Trying to ensure forget skippables before CC.

https://tbpl.mozilla.org/?tree=Try&rev=fbcd462460e4

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8521435 [details] [diff] [review]
v3

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

r- for the NS_MAJOR_FORGET_SKIPPABLE_CALLS thing but otherwise this looks good to me.

It would be nicer to just move things to GC marking, but this patch is much simpler than that so it makes sense as an interim measure.

::: dom/base/nsCCUncollectableMarker.cpp
@@ +393,5 @@
>      xulCache->MarkInCCGeneration(sGeneration);
>    }
>  #endif
>  
> +  // Keep eDone in sync with NS_MAJOR_FORGET_SKIPPABLE_CALLS.

This be some constant in a header file that both files include, and then statically assert that the number of cases here matches up.  Or something like that.

@@ +418,5 @@
> +    // frame message managers and docshells.
> +    sFSState = eInitial;
> +    return NS_OK;
> +  } else {
> +    ++sFSState;

I think you should assert that sFSSState is <= eDone afterwards.  Because it relies on how often we call this, which is not enforced in this file.

@@ +424,4 @@
>  
> +  switch(sFSState) {
> +    case eUnmarkJSEventListeners: {
> +      nsContentUtils::UnmarkGrayJSListenersInCCGenerationDocuments(sGeneration);

Do these phases all take > 1ms?

Comment 7

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

> It would be nicer to just move things to GC marking, but this patch is much
> simpler than that so it makes sense as an interim measure.
Yup



> >  
> > +  // Keep eDone in sync with NS_MAJOR_FORGET_SKIPPABLE_CALLS.
> 
> This be some constant in a header file that both files include, and then
> statically assert that the number of cases here matches up.  Or something
> like that.
> 
So is this the reason for the r-?


> @@ +418,5 @@
> > +    // frame message managers and docshells.
> > +    sFSState = eInitial;
> > +    return NS_OK;
> > +  } else {
> > +    ++sFSState;
> 
> I think you should assert that sFSSState is <= eDone afterwards.
No. The idea is that sFSSState can be also >= eDone.
I guess I could just return early when sFSSState == eDone and use the enum type, not uin32_t 

> Do these phases all take > 1ms?
Yes, at least when one has several tabs open.

Comment 8

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

Attachment: v4

Comment 9

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

uint32_t let's one to use ++, so kept that old stuff. And switch-case deals with state >= eDone

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8521565 [details] [diff] [review]
v4

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

::: dom/base/nsCCUncollectableMarker.cpp
@@ +404,5 @@
> +    eDone = 5
> +  };
> +
> +  static_assert(eDone == NS_MAJOR_FORGET_SKIPPABLE_CALLS,
> +                "eDone should be equal to NS_MAJOR_FORGET_SKIPPABLE_CALLS");

This comment isn't useful.  Maybe something like "There must be one forgetSkippable call per cleanup state" or something.  Or just remove this comment. :)

Comment 11

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

Attachment: +comment fixed

Comment 12

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

https://tbpl.mozilla.org/?tree=Try&rev=e505393c3a5b

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/683c4915bcee

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/683c4915bcee