Closed Bug 873142 Opened 8 years ago Closed 8 years ago

Use capacity instead of initializedLength when tenuring arrays

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

Attached patch v0Splinter Review
In the interpreter, using initializedLength is fine because setElement checks the capacity every time. The JITs, however, assume that the array capacity will not change between the initarray and initializing setelem opcodes. This seems like a reasonable assumption, so we should just not shrink below the existing capacity.

This also fixes a small issue where we always choose the thingKind based on the initializedLength without regard to hasDynamicElements. This would lead us to over-allocating: e.g. starting with FINALIZE_OBJECT4_BACKGROUND in the nursery, filled in 6 elements (allocating a new backing store to replace the inline elements), tenuring with FINALIZE_OBJECT8_BACKGROUND, then re-using the dynamic elements. This would leave the 8 inline elements in the heap totally unused.
Attachment #750540 - Flags: review?(wmccloskey)
Comment on attachment 750540 [details] [diff] [review]
v0

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

> In the interpreter, using initializedLength is fine because setElement checks the capacity
> every time. The JITs, however, assume that the array capacity will not change between the
> initarray and initializing setelem opcodes. This seems like a reasonable assumption, so
> we should just not shrink below the existing capacity.

That sucks, but I guess I agree.

> This also fixes a small issue where we always choose the thingKind based on the
> initializedLength without regard to hasDynamicElements. This would lead us to
> over-allocating: e.g. starting with FINALIZE_OBJECT4_BACKGROUND in the nursery,
> filled in 6 elements (allocating a new backing store to replace the inline
> elements), tenuring with FINALIZE_OBJECT8_BACKGROUND, then re-using the dynamic
> elements. This would leave the 8 inline elements in the heap totally unused.

It seems like we ought to fix this. If we have enough fixed slots, we should use them. That could be a follow-up, though.

::: js/src/gc/Nursery.cpp
@@ +228,5 @@
> +        /* Use minimal size object if we have external elements. */
> +        if (!obj->hasFixedElements())
> +            return FINALIZE_OBJECT0_BACKGROUND;
> +
> +        size_t nelements = obj->getElementsHeader()->capacity;

Just use obj->getDenseCapacity() and then you won't need to make this a member of Nursery.
Attachment #750540 - Flags: review?(wmccloskey) → review+
Reluctantly backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b54ce66659aa - one of the four things in that push, despite having been green on try, picked up causing failures in Android 2.2 reftest-1, reftest-2, and reftest-4 by the time it landed. Since I always blame the need to clobber when it's Android, I clobbered and retriggered on your push, and they still failed.
I'm going to reland this now because this patch only affects ggc enabled builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ec45bbe41a

I trychose the wrong tests apparently, but here is a green try anyway:
https://tbpl.mozilla.org/?tree=Try&rev=a704f20883bf
https://hg.mozilla.org/mozilla-central/rev/a8ec45bbe41a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.