1141234 - Refactor the allocator

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 1_move_should_fail_to_utility-v0.diff

This needs some aggressive refactoring to get us where we need to be.

Comment 1

[Terrence Cole [:terrence]]

Attachment: 2_use_unique_ptr_to_free_slots-v0.diff

Use automatic C++ memory management for the slots vector until we get it into an object.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 3_share_try_new_tenured_thing-v0.diff

We should be sharing as much as possible between object and non-object paths.

Comment 3

[Terrence Cole [:terrence]]

Attachment: 4_simplify_off_thread_alloc-v0.diff

Split out the slot+object alloc and do off-thread object allocation up front, rather than testing for isJSContext in 3 separate places.

Comment 4

[Terrence Cole [:terrence]]

Attachment: 2_use_unique_ptr_to_free_slots-v1.diff

Fixed the slots check wrong.

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8574805 [details] [diff] [review]
1_move_should_fail_to_utility-v0.diff

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

::: js/src/gc/Allocator.cpp
@@ +113,5 @@
>      if (allowGC && !rt->mainThread.suppressGC)
>          JS::AutoAssertOnGC::VerifyIsSafeToGC(rt);
>  
>      // For testing out of memory conditions
> +    if (js::oom::ShouldFailWithOOM()) {

Oops, sorry, forgot to comment -- seems like this wouldn't be a totally unreasonable place to do a MOZ_UNLIKELY?

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8574807 [details] [diff] [review]
3_share_try_new_tenured_thing-v0.diff

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

This looks like a pure refactor. Yay for eliminating dangerous duplication.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8574809 [details] [diff] [review]
4_simplify_off_thread_alloc-v0.diff

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

If you messed up anything here, I certainly don't see it. LGTM

Comment 8

[Terrence Cole [:terrence]]

Attachment: 5_inline_shouldnurseryallocate-v0.diff

There's no need to abstract this, so don't.

Comment 9

[Terrence Cole [:terrence]]

Attachment: 6_move_allocator_to_gcruntime-v0.diff

I'd like to order the allocator methods such that the CPU can read the code forward instead of backwards. Moving the declarations into GCRuntime (and getting private access to the fields we need there) seems cleaner to me than forward declaring everything.

Comment 10

[Terrence Cole [:terrence]]

Attachment: 7_reorder_allocator-v0.diff

And shuffle the tenured alloc code into order. Two code changes hidden in the moves: (1) removing a few extraneous "inline" declarations and (2) remove the allocateFromArena shim that doesn't have the AutoMaybeStartBackgroundAllocation since the Nursery can see it now.

Comment 11

[Terrence Cole [:terrence]]

Now to see what a clang opt build makes of the new code:

0000000000514250 <_ZN2js8AllocateI8JSObjectLNS_7AllowGCE1EEEPS1_PNS_16ExclusiveContextENS_2gc9AllocKindEmNS6_11InitialHeapEPKNS_5ClassE>:
_ZN2js8AllocateI8JSObjectLNS_7AllowGCE1EEEPS1_PNS_16ExclusiveContextENS_2gc9AllocKindEmNS6_11InitialHeapEPKNS_5ClassE():
/home/terrence/moz/branch/5/js/src/gc/Allocator.cpp:106
  514250:       55                      push   %rbp
  514251:       41 57                   push   %r15
  514253:       41 56                   push   %r14
  514255:       41 55                   push   %r13
  514257:       41 54                   push   %r12
  514259:       53                      push   %rbx
  51425a:       48 83 ec 18             sub    $0x18,%rsp
  51425e:       41 89 cd                mov    %ecx,%r13d
  514261:       49 89 d6                mov    %rdx,%r14
  514264:       89 f5                   mov    %esi,%ebp
  514266:       48 89 fb                mov    %rdi,%rbx
_ZN2js2gc5Arena9thingSizeENS0_9AllocKindE():
/home/terrence/moz/branch/5/js/src/gc/Heap.h:676
  514269:       89 e8                   mov    %ebp,%eax
  51426b:       48 8b 0d 66 39 56 00    mov    0x563966(%rip),%rcx        # a77bd8 <_DYNAMIC+0x4b8>
  514272:       44 8b 3c 81             mov    (%rcx,%rax,4),%r15d
_ZNK2js16ExclusiveContext11isJSContextEv():
/home/terrence/moz/branch/5/js/src/jscntxt.h:121
  514276:       83 bb a0 00 00 00 00    cmpl   $0x0,0xa0(%rbx)
  51427d:       74 1e                   je     51429d <_ZN2js8AllocateI8JSObjectLNS_7AllowGCE1EEEPS1_PNS_16ExclusiveContextENS_2gc9AllocKindEmNS6_11InitialHeapEPKNS_5ClassE+0x4d>
_ZN2js8AllocateI8JSObjectLNS_7AllowGCE1EEEPS1_PNS_16ExclusiveContextENS_2gc9AllocKindEmNS6_11InitialHeapEPKNS_5ClassE():
/home/terrence/moz/branch/5/js/src/gc/Allocator.cpp:118
  51427f:       48 89 df                mov    %rbx,%rdi
  514282:       89 ee                   mov    %ebp,%esi
  514284:       4c 89 fa                mov    %r15,%rdx
  514287:       4c 89 f1                mov    %r14,%rcx
  51428a:       48 83 c4 18             add    $0x18,%rsp
  51428e:       5b                      pop    %rbx
  51428f:       41 5c                   pop    %r12
  514291:       41 5d                   pop    %r13
  514293:       41 5e                   pop    %r14
  514295:       41 5f                   pop    %r15
  514297:       5d                      pop    %rbp

Just in the first two lines, we can see some serious badness. First, it looks like the kind->thingSize table needs a dynamic reloctaion?! Second, the off-main-thread is effectively taking a massive amount of space in the front of our common alloc path, so we'll probably want a separate ExclusiveContext version.

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8576121 [details] [diff] [review]
5_inline_shouldnurseryallocate-v0.diff

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

ShouldNurseryAllocate sounds like a cool function, until you notice that it doesn't take any info about the thing being allocated. It's more clear inline.

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8576123 [details] [diff] [review]
6_move_allocator_to_gcruntime-v0.diff

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

::: js/src/gc/Allocator.cpp
@@ +76,5 @@
>      return obj;
>  }
>  
> +bool
> +GCRuntime::gcIfNeededPerAllocation(JSContext *cx)

I don't understand the name. Is this "per" as in "for each", or "per" as in "as per" aka "due to"? gcIfNeededForAllocation?

Comment 14

[Terrence Cole [:terrence]]

It's in the first sense: how about gcIfNeededOnEachAllocation?

Comment 15

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8576127 [details] [diff] [review]
7_reorder_allocator-v0.diff

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

Lots of stuff moving around!

Comment 16

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94075dded830

Comment 17

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #14)
> It's in the first sense: how about gcIfNeededOnEachAllocation?

Ok, finally managed to parse this correctly. "This is a gcIfNeeded variant, and it's the one we call for each allocation."

In trying to come up with a better name, I keep running into the issue that there really is no good name because this isn't doing a well-defined single check. How many callers are there? I only see one in that file. Can it be inlined?

Comment 18

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #17)
> (In reply to Terrence Cole [:terrence] from comment #14)
> > It's in the first sense: how about gcIfNeededOnEachAllocation?
> 
> Ok, finally managed to parse this correctly. "This is a gcIfNeeded variant,
> and it's the one we call for each allocation."
> 
> In trying to come up with a better name, I keep running into the issue that
> there really is no good name because this isn't doing a well-defined single
> check. How many callers are there? I only see one in that file. Can it be
> inlined?

Ideally, no: I'd really, really like to have gcIfNeededPerAllocation, gcIfNeededPerArena, and gcIfNeededPerChunk all sitting right next to each other at the top of Allocator.cpp so that it is trivial to grok all of our allocation-related GC triggers. Stuffing them piecemeal all over our allocator is what we're doing now and it's a total pain to figure out what's going on.

Comment 19

[Terrence Cole [:terrence]]

Try run is at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94075dded830

OSX bustage is a known clobber issue on Try; ggc failures are know bustage from bug 1139456 and are hidden on inbound; H failures are fixed by patch 8 in this series; the rest looks like random, unrelated bustage.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eff0109392e9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65e299373b5b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/274b1f5afc29
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd5d5191e49
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8743d47b41
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3632c514a358
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df39d787c523
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7dd01f338e

Comment 20

[Terrence Cole [:terrence]]

I left the name of gcIfNeeded alone for now since we haven't decided on a new name and I'm not convinced that PerAllocation is less clear than whatever we'd come up with.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/98e5e14b05e4

https://treeherder.mozilla.org/logviewer.html#?job_id=7545795&repo=mozilla-inbound

Comment 22

[Terrence Cole [:terrence]]

I've ripped patch 2 out of the series and rebased to do the slots management manually again, which should fix both sources of bustage. Try is closed again though so I can't test; maybe I'll be able to land next week. :-/

Comment 23

[Terrence Cole [:terrence]]

Unexpected, but welcome: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da1f4735c628

Comment 24

[Terrence Cole [:terrence]]

Failed the merge, let's try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77979cb9008e

Comment 25

[Terrence Cole [:terrence]]

Try run is totally green and no last minute changes this time:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77979cb9008e

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/169996b1c77a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/444f904de5a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e14cd78539b0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c11057814b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/43a8181e6e2c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/67b1737a236d

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/169996b1c77a
https://hg.mozilla.org/mozilla-central/rev/444f904de5a7
https://hg.mozilla.org/mozilla-central/rev/e14cd78539b0
https://hg.mozilla.org/mozilla-central/rev/7c11057814b5
https://hg.mozilla.org/mozilla-central/rev/43a8181e6e2c
https://hg.mozilla.org/mozilla-central/rev/67b1737a236d