Last Comment Bug 777919 - Free LifoAlloc chunks on background thread
: Free LifoAlloc chunks on background thread
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Bill McCloskey (:billm)
: general
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-26 14:09 PDT by Bill McCloskey (:billm)
Modified: 2012-07-27 08:58 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.29 KB, patch)
2012-07-26 14:10 PDT, Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-07-26 14:09:43 PDT
On Macs, we spend a lot of time freeing LifoAlloc chunks. This patch moves that activity to the background GC thread.

I also stopped calling PurgeRuntime from the write barrier verifier. It messes things up for this patch, and it's no longer needed.
Comment 1 Bill McCloskey (:billm) 2012-07-26 14:10:17 PDT
Created attachment 646330 [details] [diff] [review]
patch
Comment 2 Luke Wagner [:luke] 2012-07-26 14:10:48 PDT
What sort of speedup do you see?
Comment 3 Bill McCloskey (:billm) 2012-07-26 14:16:58 PDT
(In reply to Luke Wagner [:luke] from comment #2)
> What sort of speedup do you see?

Well, I've seen GCs where we spend over a second doing these frees (and nothing else). Now we'll be doing them on a background thread. I don't have a Mac, so I haven't verified anything directly.
Comment 4 Andrew McCreight [:mccr8] 2012-07-26 14:28:57 PDT
This takes up a huge amount of time for me

From a random GC I had:
  Free TI Arena: 55.2ms

Bill, if you throw up a try build I can test this out, as I seem to be the king of awful LifoAlloc freeing.

Slice: 22, Pause: 147.3 (When: 2463.1ms, Reason: INTER_SLICE_GC): Mark: 8.6ms, Mark Weak: 0.4ms, Mark Gray: 7.8ms, Mark Gray and Weak: 0.3ms, Finalize Start Callback: 0.9ms, Sweep: 121.2ms, Sweep Atoms: 11.6ms, Sweep Compartments: 85.4ms, Sweep Tables: 17.6ms, Sweep Object: 3.8ms, Sweep String: 0.5ms, Sweep Script: 1.2ms, Sweep Shape: 10.2ms, Sweep Discard Code: 2.4ms, Discard Analysis: 64.9ms, Discard TI: 0.6ms, Free TI Arena: 55.2ms, Sweep Types: 8.1ms, Clear Script Analysis: 0.8ms, Finalize End Callback: 6.3ms, Deallocate: 0.6ms, End Callback: 14.9ms
Comment 5 Bill McCloskey (:billm) 2012-07-26 14:42:37 PDT
Try build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wmccloskey@mozilla.com-58bf5b05737a/

Reviewing Andrew's GCs over the last 4 hours, there are at least twenty that spend over 100ms calling free, and some that are over 250ms.
Comment 6 Andrew McCreight [:mccr8] 2012-07-26 14:47:28 PDT
Okay, I'm running the new build now.  I changed the email address to amccreightFastLIFO@mozilla.com in your GC2 addon so you can monitor me remotely.
Comment 7 Bill McCloskey (:billm) 2012-07-26 14:49:15 PDT
(In reply to Andrew McCreight [:mccr8] from comment #6)
> Okay, I'm running the new build now.  I changed the email address to
> amccreightFastLIFO@mozilla.com in your GC2 addon so you can monitor me
> remotely.

OK, thanks. It logs the buildid, though, so that shouldn't be necessary.
Comment 8 Andrew McCreight [:mccr8] 2012-07-26 15:05:44 PDT
Oh, makes sense!

Here's what it looks like now, though this isn't a fair comparison because my other session had been up for a day or so I think:

    Slice: 11, Pause: 49.8 (When: 1220.5ms, Reason: INTER_SLICE_GC): Mark: 5.2ms, Mark Weak: 0.1ms, Mark Gray: 4.8ms, Mark Gray and Weak: 0.1ms, Finalize Start Callback: 0.3ms, Sweep: 42.6ms, Sweep Atoms: 10.1ms, Sweep Compartments: 20.5ms, Sweep Tables: 7.1ms, Sweep Object: 3.0ms, Sweep String: 0.3ms, Sweep Script: 0.9ms, Sweep Shape: 5.3ms, Sweep Discard Code: 2.3ms, Discard Analysis: 11.0ms, Discard TI: 3.9ms, Sweep Types: 6.2ms, Clear Script Analysis: 0.7ms, Finalize End Callback: 1.7ms, Deallocate: 0.3ms, End Callback: 1.0ms

Patch of the year!
Comment 9 Luke Wagner [:luke] 2012-07-26 16:07:13 PDT
Comment on attachment 646330 [details] [diff] [review]
patch

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

Sounds great, patch looks good.

::: js/src/ds/LifoAlloc.cpp
@@ +63,5 @@
> +    first = latest = last = NULL;
> +}
> +
> +LifoAlloc::BumpChunk *
> +LifoAlloc::getLastUsed()

This can go also.

@@ +172,5 @@
> +        return;
> +
> +    /* Rewind through any unused chunks. */
> +    if (!other->latest->used()) {
> +        other->latest = other->getLastUsed();

As discussed, there should only be unused chunks while compilation is active which means we probably don't expect to win very much from this case so it could probably be removed.  Also the case in freeUnused.

::: js/src/ds/LifoAlloc.h
@@ +188,5 @@
>          other->reset(defaultChunkSize_);
>      }
>  
> +    /* Append allocated chunks from |other|. They are removed from |other|. */
> +    void transfer(LifoAlloc *other);

Could you name this transferFrom to make the direction very clear?

@@ +191,5 @@
> +    /* Append allocated chunks from |other|. They are removed from |other|. */
> +    void transfer(LifoAlloc *other);
> +
> +    /* Append unused chunks from |other|. They are removed from |other|. */
> +    void transferUnused(LifoAlloc *other);

ditto

Note You need to log in before you can comment on or make changes to this bug.