The default bug view has changed. See this FAQ.

Free LifoAlloc chunks on background thread

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 646330 [details] [diff] [review]
patch
Attachment #646330 - Flags: review?(luke)

Comment 2

5 years ago
What sort of speedup do you see?
(Assignee)

Comment 3

5 years ago
(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.
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
Whiteboard: [Snappy]
(Assignee)

Comment 5

5 years ago
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.
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.
(Assignee)

Comment 7

5 years ago
(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.
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

5 years ago
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
Attachment #646330 - Flags: review?(luke) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/05235eee2b6b
https://hg.mozilla.org/mozilla-central/rev/05235eee2b6b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.