888313 - GenerationalGC: Investigate earley-boyer performance

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Hackish patch

Terrence was interested in doing GGC performance work in parallel with fuzzing, so I'm filing this bug to collect data about tuning GGC for v8-earley.

Current scores (--ion-parallel-compile=on run-earley-boyer.js):

ggc build   : 18600
no-ggc build: 23273
d8          : 36983

Here's a number of tweaks/optimizations to improve this:

(*) Changing the shrink/grow-the-nursery thresholds from 0.1/0.5 to 0.01/0.05 allows the nursery to actually grow to 8 MB. These numbers are from bug 851057, we should try different values and see what happens.

New score: 25009

(*) Setting the nursery size to 16 MB reduces the GC time and number of tenured objects. I'm not sure about this one, it makes some sense since our Values are twice as large as V8's. OTOH, maybe the underlying problem is that we are allocating too much or should pretenure more.

New score: 27772

(*) Replacing JS_TraceChildren with handwritten tracing code. Most of the code is from Brian's patch in bug 851057.

New score: 30553

So these close half the gap with V8 on earley-boyer. We still spend more time GCing than V8 (and have more GCs), we should find out why. I also wonder if we're allocating more data than them (call objects, function clones?)

Attaching a (hackish) patch implementing these changes.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Avoid store buffer overflow in pdfjs.js.

I was just profiling pdfjs.js under GGC when I stumbled across this bug.  With
the attached patch that changes only CellBufferSize, I avoided all the store 
buffer overflows and reduced the slowdown from ~1.77x to ~1.10x.

Comment 2

[Nicholas Nethercote [inactive]]

(Apologies for hijacking the bug, BTW;  I can file a new bug if my patch gets r+.)

Comment 3

[Nicholas Nethercote [inactive]]

Octane scores before and after my patch:

Richards: 15476
DeltaBlue: 10341
Crypto: 21491
RayTrace: 39328
EarleyBoyer: 16720
RegExp: 2910
Splay: 7572
NavierStokes: 28231
Mandreel: 5379
Gameboy: 3346
CodeLoad: 24477
Box2D: 7123

Richards: 18474
DeltaBlue: 22890
Crypto: 21643
RayTrace: 38997
EarleyBoyer: 16437
RegExp: 2901
Splay: 7483
NavierStokes: 27964
Mandreel: 5338
Gameboy: 3344
CodeLoad: 24559
Box2D: 7312

Big improvements in Richrads and DeltaBlue.  Pdfjs isn't there because there was some kind of error ("Invalid font names") in both runs.

Comment 4

[Jan de Mooij [:jandem]]

Very nice! Bug 887322 comment 1 also has some info on pdf.js. I'm surprised you didn't have to tweak the SlotBufferSize and GenericBufferSize like I did there though, maybe something changed recently..

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 774489 [details] [diff] [review]
Avoid store buffer overflow in pdfjs.js.

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

The fuzz bug 889682 uncovered a problem with our fallback marking that is pretty much unsolvable, so I'm actually rewriting all of these to be "infinite" sized at the moment.

Despite the fact that I'm going to be ripping it out, this work is excellent: it tells me exactly where I should set the default chunk size and high-water marks without having to do the legwork myself. Thanks! r=me

Comment 6

[Terrence Cole [:terrence]]

(In reply to Jan de Mooij [:jandem] from comment #0)
>
> (*) Changing the shrink/grow-the-nursery thresholds from 0.1/0.5 to
> 0.01/0.05 allows the nursery to actually grow to 8 MB. These numbers are
> from bug 851057, we should try different values and see what happens.
> 

I think the underlying problem here is a rather silly bug in |put|. When we go to detect the about-to-overflow state, we don't compact first. Thus we set about-to-overflow, but before we do the minor GC we compact, so we end up doing minor GC's with an empty nursery and an empty store buffer very frequently. Fixing this is a huge boost to our scores across the board (but see bug 889682 before jumping on this).

> 
> (*) Setting the nursery size to 16 MB reduces the GC time and number of
> tenured objects. I'm not sure about this one, it makes some sense since our
> Values are twice as large as V8's. OTOH, maybe the underlying problem is
> that we are allocating too much or should pretenure more.
> 
> New score: 27772

This I totally believe.
 
> (*) Replacing JS_TraceChildren with handwritten tracing code. Most of the
> code is from Brian's patch in bug 851057.
> 
> New score: 30553

I will be very sad if we can't share this with MarkChildren/PushMarkStack somehow.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Terrence Cole [:terrence] from comment #5)
> Comment on attachment 774489 [details] [diff] [review]
> Avoid store buffer overflow in pdfjs.js.
> 
> Review of attachment 774489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The fuzz bug 889682 uncovered a problem with our fallback marking that is
> pretty much unsolvable, so I'm actually rewriting all of these to be
> "infinite" sized at the moment.

Good!  I see you wrote the following in bug 889682 comment 4:

> (1) We're always going to run into a performance cliff of this
> magnitude if we don't switch to a growable store buffer.

and I was having similar worries about performance cliffs.

Comment 8

[Nicholas Nethercote [inactive]]

I just filed bug 893158 for my patch.  This bug can revert to being about Jan's patch! :)

Comment 9

[Terrence Cole [:terrence]]

Attachment: v0: tweak the GGC tuning

The number tweaks from this patch (I think the last bit that hasn't been moved to a different bug) still appear to make sense:

before: 10295
after:  10630
tip:    10810
d8:     12894

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 777919 [details] [diff] [review]
v0: tweak the GGC tuning

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

Nice.

Comment 11

[Terrence Cole [:terrence]]

Attachment: tweak_ggc_params-vCheckin.diff

Comment 12

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3a1d390324

I think there's still more to do here.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea3a1d390324

Comment 14

[Terrence Cole [:terrence]]

I'm closing the GGC opt bugs; I think we've worked through all the major issues in more targeted bugs.