1134425 - Out-of-line our allocator

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 1.0_split_out_allocator_and_clean_up-v0.diff

It appears that, despite some pretty large changes, the tenured allocation path is still fully inlinable all the way from Object::create down to the actual bump allocation in gc/Heap.h. Of course, given that nursery allocation in C++ has never been inlinable, the chance that this property is useful is nil. It certainly doesn't help code quality.

In any case, it's well past time we moved this all out of line.

Additionally, I've simplified our allocation paths with C++, so that instead of 12 different entry points we have a single templated "Allocate" method. I had intended to do this as a second pass, but it turns out mq falls over hard on dealing with files that aren't in the repo. This actually gets rid of a pretty ugly intermediate state, so it's not so bad in practice.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8566255 [details] [diff] [review]
1.0_split_out_allocator_and_clean_up-v0.diff

Actually, I guess it's Jon's turn, given how many I sent Steve's way last week.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 1.0_split_out_allocator_and_clean_up-v1.diff

Looks like Allocator.cpp got dropped during a merge.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8566267 [details] [diff] [review]
1.0_split_out_allocator_and_clean_up-v1.diff

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

This is a really nice reorganisation!

My comments are minor and based on the original code not your changes.

::: js/src/gc/Allocator.cpp
@@ +140,5 @@
> +    if (!cx->isJSContext())
> +        return;
> +
> +    Zone *zone = cx->asJSContext()->zone();
> +    MOZ_ASSERT_IF(t && zone->wasGCStarted() && (zone->isGCMarking() || zone->isGCSweeping()),

zone->wasGCStarted() is redundant given the other checks

@@ +150,5 @@
> +JSObject *
> +js::Allocate(ExclusiveContext *cx, AllocKind kind, size_t nDynamicSlots, InitialHeap heap,
> +             const Class *clasp)
> +{
> +    (void)static_cast<JSObject *>((T*)0);

I think I originally came up with this, but if possible something like this might be clearer:

  static_assert(mozilla::IsConvertible<T*, JSObject*>::value, "must be JSObject derived")

@@ +242,5 @@
> +
> +template <typename T, AllowGC allowGC>
> +T *
> +js::Allocate(ExclusiveContext *cx)
> +{

Maybe we should assert T is not derived from JSObject here.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> Comment on attachment 8566267 [details] [diff] [review]
> 1.0_split_out_allocator_and_clean_up-v1.diff
> 
> Review of attachment 8566267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a really nice reorganisation!
> 
> My comments are minor and based on the original code not your changes.
> 
> @@ +150,5 @@
> > +JSObject *
> > +js::Allocate(ExclusiveContext *cx, AllocKind kind, size_t nDynamicSlots, InitialHeap heap,
> > +             const Class *clasp)
> > +{
> > +    (void)static_cast<JSObject *>((T*)0);
> 
> I think I originally came up with this, but if possible something like this
> might be clearer:
> 
>   static_assert(mozilla::IsConvertible<T*, JSObject*>::value, "must be
> JSObject derived")

Good idea! I had forgotten about IsConvertible. I do <3 the static_cast trick, however.

> @@ +242,5 @@
> > +
> > +template <typename T, AllowGC allowGC>
> > +T *
> > +js::Allocate(ExclusiveContext *cx)
> > +{
> 
> Maybe we should assert T is not derived from JSObject here.

Good idea! I did verify that trying to Allocate<JSObject, allowGC>(cx) fails at link time because of the missing instantation, but this will make the error clearer.

Comment 5

[Terrence Cole [:terrence]]

Attachment: 1_create_allocator-v1.diff

I didn't land because of multiple days of tree closure, in which time something changed in jsgcinlines.h and merging became basically impossible. So I've just rewritten it, this time in parts as intended. I'm carrying the r+.

Comment 6

[Terrence Cole [:terrence]]

Attachment: 2_templatize_allocations-v1.diff

And the allocator change -- done in a different tree this time to ensure that my mq patch had all the right bits in it. Now that these are separate patches, we can see that templatizing the allocator saves exactly 98 lines. \o/

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9dfca903cc
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b916102eedb

Comment 8

[Terrence Cole [:terrence]]

I'm landing this in separate pieces to minimize danger, given that I've had nasty merge conflicts twice now on the first part.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c9dfca903cc

Comment 10

[Terrence Cole [:terrence]]

And the second half is green too:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b78a8f4ab3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1658c12a950d

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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