Closed Bug 897433 Opened 7 years ago Closed 7 years ago

Telemetry for SnowWhite and more async SnowWhite freeing

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(3 files, 3 obsolete files)

Attached 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+
(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.
Attached 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)
Attached 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.
Attached 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+
Attached 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: 7 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.