897433 - Telemetry for SnowWhite and more async SnowWhite freeing

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Attachment: patch

I noticed that avg max forgetskippable times have gone up a tiny bit (1ms or so) since
SW landing.

Comment 1

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

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

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 780326 [details] [diff] [review]
patch

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

::: xpcom/base/nsCycleCollector.cpp
@@ +2236,5 @@
> +            if (!mAsyncSnowWhiteFreeing) {
> +                SnowWhiteKiller::Visit(aBuffer, aEntry);
> +            } else if (!mDispatchedDeferredDeletion) {
> +                mDispatchedDeferredDeletion = true;
> +                nsCycleCollector_dispatchDeferredDeletion();

So, this dispatchDeferredDeletion just queues up something to run, but we won't actually clear anything until later?  I guess in RemoveSkippableVisitor we don't care if we leave snow white objects around in the purple buffer.

::: xpcom/base/nsCycleCollector.h
@@ +53,5 @@
>  
>  typedef void (*CC_ForgetSkippableCallback)(void);
>  void nsCycleCollector_setForgetSkippableCallback(CC_ForgetSkippableCallback aCB);
>  
> +void nsCycleCollector_forgetSkippable(bool aRemoveChildlessNodes = false,

Having two bool arguments in a row is a little gross, but ok.

Comment 3

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ededcd9b11f

Comment 4

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

This got merged to m-c and immediately backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/a4c1961bf723

Comment 5

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

(In reply to Wes Kocher (:KWierso) from comment #4)
> This got merged to m-c and immediately backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/a4c1961bf723

Hit submit too soon. 
Backed out for causing frequent dromaeo crashes on Linux32 like https://tbpl.mozilla.org/php/getParsedLog.php?id=25690942&tree=Mozilla-Inbound&full=1

Comment 6

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

Whaat.

Comment 7

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

Oh crap, OOM. Really odd.

Comment 8

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

Or no. I missed that we don't know do the second sw killing after forget skippable.

Comment 9

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

Attachment: alternative, v2

If we delete some sw objects, run sw killer soon again, once.
This is perhaps temporary solution. Trying to figure out something better.

interdiff coming

Comment 10

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

Attachment: interdiff

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/300f5b7d72e1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks like dromaeo is still timing out (albeit at a lower rate than before) :(
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=300f5b7d72e1

Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9fad4b940f

Comment 13

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

How odd. Really really odd.

Comment 14

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

(I had pushed this to try)

Comment 15

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

And the stack in that tbpl log isn't too useful.

Comment 16

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

Ahaa, Dromaeo's DOM Modification test takes lots of memory, even if we kill SnowWhite objects all the
time.

And memory usage is around the same even on Aurora which doesn't have snow-white.

Investigating...

Comment 17

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

So, we use tons of memory temporary even in branches because the test creates lots of new
nodes and each of them gets a wrapper. The wrapper keeps the node alive. Then, we may gc during the
tight loop, but nodes are released asynchronously.
I don't see much difference with SW, but seems like we're almost OOM even without it, and then
just tiny bit more memory usage is enough to push us to the boundary.

Comment 18

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

Er, no, we do release some of the stuff right after a GC if we have still pending deferred releases from the previous GC. But even then it is possible to keep tons of objects alive for some time.

Comment 19

[Andrew McCreight [:mccr8]]

Maybe the deferred release mechanism could FreeSnowWhite() when it is done?  That way if we're heavily GCing we'll be sure to free anything released by it.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Olli Pettay [:smaug] from comment #14)
> (I had pushed this to try)

Yeah, it's not a perma-fail. You'll want to retrigger it 15-20 times to get a sense for whether or not you're OK.

Comment 21

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

Attachment: v3

interdiff coming.

The main change is to trigger sw killer also after non-iGC (JS::GC_CYCLE_END).
Other change is to not have several AsyncFreeSnowWhite objects alive, but
just use one per cycle collector, and don't redispatch it if one is active.
But possibly change continuation bit so that we get enough snowwhite killing.

Comment 22

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

Attachment: snow_white_telemetry_2_3.interdiff

Comment 23

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

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

Comment 24

[Andrew McCreight [:mccr8]]

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

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

r=me with the comments addressed.

::: dom/base/nsJSEnvironment.cpp
@@ +3074,5 @@
>        nsJSContext::PokeShrinkGCBuffers();
>      }
>    }
>  
> +  if ((aProgress == JS::GC_SLICE_END || aProgress == JS::GC_CYCLE_END) &&

Oops, I forgot about that.

::: xpcom/base/nsCycleCollector.cpp
@@ +999,5 @@
>  
>  public:
>      nsCycleCollectorParams mParams;
>  
> +    nsRefPtr<AsyncFreeSnowWhite> mAsyncSnowWhiteFreer;

This field is really odd.  It is initially null, but then you lazily initialize it and never clear it?  It seems better to me to just initialize it at startup, then make it private and add a getter method for Dispatch() to use.

@@ +2161,5 @@
> +{
> +public:
> +  NS_IMETHOD Run()
> +  {
> +      PRTime start = PR_Now();

Please use TimeStamp instead of PRTime in this file, like TimeLog::CheckPoint does.

Comment 25

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

Attachment: v4

Comment 26

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

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

Try looked good. Pushed to m-c since m-i is again closed.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

/me grumbles about merging through this.