777919 - Free LifoAlloc chunks on background thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 [inactive unless it's an emergency] (:billm)]

Attachment: patch

Comment 2

[Luke Wagner [:luke]]

What sort of speedup do you see?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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]]

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 [inactive unless it's an emergency] (:billm)]

Try build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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]]

Okay, I'm running the new build now.  I changed the email address to [e-mail] in your GC2 addon so you can monitor me remotely.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Okay, I'm running the new build now.  I changed the email address to
> [e-mail] 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]]

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]]

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

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05235eee2b6b

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/05235eee2b6b