Investigate if we could have more major forget skippable runs

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

36 Branch
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Created attachment 8520963 [details] [diff] [review]
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 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.
Attachment #8520963 - Flags: review?(continuation)
Comment on attachment 8520963 [details] [diff] [review]
v1

Actually, I want to add yet another phase.
Attachment #8520963 - Flags: review?(continuation)
Created attachment 8521409 [details] [diff] [review]
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.
Attachment #8521409 - Flags: review?(continuation)
Hmm, no, not good. We get sync forgetSkippables too often.
Created attachment 8521435 [details] [diff] [review]
v3

Trying to ensure forget skippables before CC.

https://tbpl.mozilla.org/?tree=Try&rev=fbcd462460e4
Attachment #8520963 - Attachment is obsolete: true
Attachment #8521409 - Attachment is obsolete: true
Attachment #8521409 - Flags: review?(continuation)
Attachment #8521435 - Flags: review?(continuation)
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?
Attachment #8521435 - Flags: review?(continuation) → review-
> 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.
Created attachment 8521565 [details] [diff] [review]
v4
Attachment #8521565 - Flags: review?(continuation)
uint32_t let's one to use ++, so kept that old stuff. And switch-case deals with state >= eDone
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. :)
Attachment #8521565 - Flags: review?(continuation) → review+
Created attachment 8521585 [details] [diff] [review]
+comment fixed
Attachment #8521435 - Attachment is obsolete: true
Attachment #8521565 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/683c4915bcee
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.