Out-of-line our allocator

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks 1 bug)

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Attachment #8566255 - Flags: review?(sphink)
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.
Attachment #8566255 - Flags: review?(sphink) → review?(jcoppeard)
Looks like Allocator.cpp got dropped during a merge.
Attachment #8566255 - Attachment is obsolete: true
Attachment #8566255 - Flags: review?(jcoppeard)
Attachment #8566267 - Flags: review?(jcoppeard)
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.
Attachment #8566267 - Flags: review?(jcoppeard) → review+
(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.
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+.
Attachment #8566267 - Attachment is obsolete: true
Attachment #8572729 - Flags: review+
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/
Attachment #8572764 - Flags: review+
I'm landing this in separate pieces to minimize danger, given that I've had nasty merge conflicts twice now on the first part.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/b6b78a8f4ab3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.