Telemetry for SnowWhite and more async SnowWhite freeing

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla25
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
I noticed that avg max forgetskippable times have gone up a tiny bit (1ms or so) since
SW landing.
Attachment #780326 - Flags: review?(continuation)
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.
Attachment #780326 - Flags: review?(continuation) → review+
This got merged to m-c and immediately backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/a4c1961bf723
(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
Whaat.
Oh crap, OOM. Really odd.
Or no. I missed that we don't know do the second sw killing after forget skippable.
Posted patch alternative, v2 (obsolete) — Splinter Review
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
Attachment #781015 - Flags: review?(continuation)
Posted patch interdiff (obsolete) — Splinter Review
Attachment #781015 - Flags: review?(continuation) → review+
How odd. Really really odd.
(I had pushed this to try)
And the stack in that tbpl log isn't too useful.
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...
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.
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.
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.
(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.
Posted patch v3Splinter Review
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.
Attachment #780326 - Attachment is obsolete: true
Attachment #781015 - Attachment is obsolete: true
Attachment #781017 - Attachment is obsolete: true
Attachment #781907 - Flags: review?(continuation)
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.
Attachment #781907 - Flags: review?(continuation) → review+
Posted patch v4Splinter Review
https://hg.mozilla.org/mozilla-central/rev/a2c4d9491491

Try looked good. Pushed to m-c since m-i is again closed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
/me grumbles about merging through this.
You need to log in before you can comment on or make changes to this bug.