957542 - Don't over-allocate dynamic slots of result object of re.exec()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

When creating the dynamic slots for the result array of re.exec() we need 2 dynamic slots (1 for index, 1 for input). Now we have "SLOT_CAPACITY_MIN" that defines that we must have 8 slots minimum. So we are always allocating 8 slots instead of 2 for the result array. This is very costly. Removing this restriction increases our score on octane2.0-regexp from 2550 to 2760 (8% increase).

So what is the idea behind "SLOT_CAPACITY_MIN" and do we need it? And where do we use it? If we need it, can I short circuit createArray or should I short circuit CreateRegExpMatchResult.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch used to get benchmark score

Comment 2

[Hannes Verschore [:h4writer]]

*another idea discussed on IRC with jandem is to get these dynamics slots inlined. 

1968x 1 elem
455x 2 elems
694x 3 elems
159x 4 elems
110x 5 elems
60x 6 elems
6x 7 elems
105x 10 elems
549x 12 elems

There are 31 inline slots. 2 taken for element header. So 29 open inline slots. We need 2 slots for input/index. So 27 open inline slots would still allow us to have all elements in the inline slots.

So currently the inlined slots are:
[elem_header1, elem_header2, element1, element2, element3 ...]

It would get changed to:
[elem_header1, elem_header2, input, index, element1, element2, element3 ...]

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: bug957542-inline-fixedslots

This is a nice 8% improvement on octane-regexp and makes bug 957570 much easier.

So the order is:
[fixed_slot_1, fixed_slot_2, ..., header_part_1, header_part_2, fixed_element_1, ...]

Now this only works with ArrayObject atm. I first tried to make it more globally. That way all natives could have slots and elements. BUT currently ArrayBuffer doesn't differentiate between slots/elements (unintentional). This is because in the inners of NewObject the number of fixed slots is set to the maximum of the allocKind. Though ArrayBuffer only has elements, it will report to have "numFixedSlots" equal to capacity. This code is quite deep and would require some changes on the arguments of NewObject etc. Would make this patch bigger than intended, with no gain, but correctness on something nobody cared for already 2 years.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8364270 [details] [diff] [review]
bug957542-inline-fixedslots

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

Not having external slots/elements is nice and should also help other regexp-heavy code like jQuery stuff.

Looks good overall; comments are mostly nits. The numFixedSlots() thing is more serious though. Also, it'd be good to get review from bhackett on this one or, since he's away, maybe terrence or Waldo.

::: js/src/jsarray.cpp
@@ +3245,5 @@
> +ArrayObject * JS_FASTCALL
> +js::NewDenseEmptyArrayWithFixedSlots(JSContext *cx, uint32_t nfixed, JSObject *proto /* = nullptr */,
> +                                     NewObjectKind newKind /* = GenericObject */)
> +{
> +    return NewArray<false>(cx, 2, 0, proto, newKind);

s/2/nfixed

::: js/src/jsgc.h
@@ +251,5 @@
>      /*
> +     * Dense arrays must at least be able to store fixed slots and an elements
> +     * header (which is two Values worth of ObjectElements header). When there
> +     * is still enough place the elements are also stored inline, else they are
> +     * stored dynamically. In the later case there is no need to reserve place

Nit: s/later/latter, s/place/space

::: js/src/jsobjinlines.h
@@ +547,5 @@
>      obj->setFixedElements();
> +
> +    // Set 0 as capacity and immediatly fix it to compute the right capacity value.
> +    new (obj->getElementsHeader()) js::ObjectElements(0, length);
> +    (&obj->as<js::ArrayObject>())->fixCapacity(kind, shape->numFixedSlots(), length);

Nit: obj->as<js::ArrayObject>.fixCapacity(...)

::: js/src/vm/ArrayObject.h
@@ +34,5 @@
>      }
> +
> +    void fixCapacity(gc::AllocKind kind, uint32_t nfixed, uint32_t nelems) {
> +        uint32_t space = js::gc::GetGCKindSlots(kind);
> +        JS_ASSERT(space >= nfixed + 2);

Nit: s/2/ObjectElements::VALUES_PER_HEADER here and below.

::: js/src/vm/ObjectImpl.cpp
@@ +247,5 @@
> +js::ObjectImpl::fixedElements() const
> +{
> +    static_assert(2 * sizeof(Value) == sizeof(ObjectElements),
> +            "when elements are stored inline, the first two "
> +            "slots will hold the ObjectElements header");

Nit: you can intead s/2/ObjectElements::VALUES_PER_HEADER below.

@@ +251,5 @@
> +            "slots will hold the ObjectElements header");
> +
> +    // ArrayObject has fixed slots and header before the elements.
> +    if (type_->clasp() == &js::ArrayObject::class_)
> +        return &fixedSlots()[numFixedSlots() + 2];

Would it work to do this for all objects?

@@ +362,2 @@
>      if (static_cast<const JSObject *>(this)->is<ArrayObject>())
> +        return numFixedSlots();

I don't think this is safe: numFixedSlots() uses the object's shape and the main thread could be giving the object a new shape etc. Is this method called for arrays? If not we could assert it's not, maybe.

Also, should we fix other callers of GetGCKindSlots, like NumFixedSlots in IonBuilder.cpp? Maybe it should assert the class is not the ArrayObject class?

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Hannes Verschore [:h4writer] from comment #0)
> So what is the idea behind "SLOT_CAPACITY_MIN" and do we need it? And where
> do we use it? If we need it, can I short circuit createArray or should I
> short circuit CreateRegExpMatchResult.

The main place where SLOT_CAPACITY_MIN will be relevant is in dynamicSlotsCount().  Right now as an object is given properties the first dynamic property will give it a SLOT_CAPACITY_MIN array, and as it grows beyond that its capacity doubles each time.  So without SLOT_CAPACITY_MIN we will allocate and reallocate slot buffers with capacities of 1, 2, 4, 8, 16, 32, 64, ... dynamic properties, whereas with SLOT_CAPACITY_MIN == 8 we will only use capacities 8, 16, 32, 64, ... which will avoid a lot of malloc traffic for objects with small numbers of dynamic properties (which there can be a lot of).

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> I don't think this is safe: numFixedSlots() uses the object's shape and the
> main thread could be giving the object a new shape etc. Is this method
> called for arrays? If not we could assert it's not, maybe.

numFixedSlotsForCompilation() can be called for arrays (it gets used by main thread APIs that are also used off thread) just as it can be for nursery objects which the compilation thread doesn't have access to.  Right now though we shouldn't call numFixedSlotsForCompilation() from off thread for any arrays whose shape could change --- due to some old quirk we never give arrays singleton type, so the only arrays used during compilation are immutable template objects, which numFixedSlots() can be called on off thread.  Pretty brittle but this is ok for now at least.

> Also, should we fix other callers of GetGCKindSlots, like NumFixedSlots in
> IonBuilder.cpp? Maybe it should assert the class is not the ArrayObject
> class?

Hmm, NumFixedSlots in IonBuilder.cpp should be replaced with calls to numFixedSlotsForCompilation.

Comment 7

[Hannes Verschore [:h4writer]]

Thanks for the explanation!

(In reply to Brian Hackett (:bhackett) from comment #6)
> due to some old quirk we never give
> arrays singleton type, so the only arrays used during compilation are
> immutable template objects, which numFixedSlots() can be called on off
> thread.  Pretty brittle but this is ok for now at least.

Oh, so temporary this would work. Though seems fragile. Since terrence landed a much easier/better fix for GGC and we will go to GGC shortly I think we can abandon the approach of trying to fit elements and fixed slots together.


(In reply to Brian Hackett (:bhackett) from comment #5)
> SLOT_CAPACITY_MIN == 8 we will
> only use capacities 8, 16, 32, 64, ... which will avoid a lot of malloc
> traffic for objects with small numbers of dynamic properties (which there
> can be a lot of).

Oh so we can't decrease this limit globally. I'll look into creating a shortcut for the result of re.exec to be 2.

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: bug957542-lower-dynamic-slotcount

If I understand correctly SLOT_CAPACITY_MIN is mostly important for normal objects. Since we expect that ArrayObjects don't have slots often, except for the case of RegExpResult, that limit doesn't help. It even deteriorate performance. This patch makes SLOT_CAPACITY_MIN only be used for normal objects and ignores it for arrays.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 8372308 [details] [diff] [review]
bug957542-lower-dynamic-slotcount

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

::: js/src/vm/ObjectImpl.cpp
@@ +338,5 @@
> +js::ObjectImpl::numDynamicSlots() const
> +{
> +    bool isArrayObject = static_cast<const JSObject *>(this)->is<ArrayObject>();
> +    return dynamicSlotsCount(numFixedSlots(), slotSpan(), isArrayObject);
> +}

Maybe put this in an -inl.h file?

::: js/src/vm/ObjectImpl.h
@@ +1407,5 @@
>       * an object with the given number of fixed slots and slot span. The slot
>       * capacity is not stored explicitly, and the allocated size of the slot
> +     * array is kept in sync with this count. For arrayObjects the count is
> +     * not increased to SLOT_CAPACITY_MIN. Slots are not that common in that
> +     * case.

I think the last two sentences here would look better as a comment by the !arrayObject test below.

@@ +1412,2 @@
>       */
> +    static uint32_t dynamicSlotsCount(uint32_t nfixed, uint32_t span, bool arrayObject) {

I think this interface would be better if it took a Clasp* instead of a boolean, both to allow easier tuning in the future and to be more consistent with other object APIs.

Comment 10

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7aa3047ec7

Comment 11

[Hannes Verschore [:h4writer]]

Fix for GGC:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2df850200c16

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for jit-test crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f074c33ad6a

https://tbpl.mozilla.org/php/getParsedLog.php?id=34416024&tree=Mozilla-Inbound

Comment 13

[Ryan VanderMeulen [:RyanVM]]

And the other half of that too:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50a87548c0e

FTR, comment 12 was on the push with the GGC fix.

Comment 14

[Hannes Verschore [:h4writer]]

Looks like the assert wasn't correct.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a474ee48949

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a474ee48949