Bug 211279 – Merge putLength() into setLength()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Seems superfluous and costly to do the method table dispatch.

[Alexey Shvayka]

Created attachment 407289 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #1)
> Created attachment 407289 [details]
> Patch

Warmed-up runs, --outer 50:

                                    r266157                   patch

array-shift-unshift-empty       64.8823+-1.0893     ^     45.5567+-1.4411        ^ definitely 1.4242x faster

[Saam Barati]

Comment on attachment 407289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407289&action=review

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:874
> +    JSValue result = thisObj->get(globalObject, index);

Isn’t the whole point of the previous code that we need the Identifier when at the MAX_ARRAY_INDEX boundary?

[Darin Adler]

Comment on attachment 407289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407289&action=review

Ah, the runtime. One area of JavaScriptCore that I still have enough expertise in that I can review.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:166
> +    if (LIKELY(isJSArray(obj))) {

Obviously this *is* likely, but I do like to follow the principle of only adding these when we know they’re beneficial so not 100% sure I would have added it. Hard to argue against it strongly, though.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:874
>> +    JSValue result = thisObj->get(globalObject, index);
> 
> Isn’t the whole point of the previous code that we need the Identifier when at the MAX_ARRAY_INDEX boundary?

Perhaps, but since Object::get(JSObject*, uint64_t) takes care of creating the identifier when it’s needed, this code will work properly without separate branches for valid array index values and other values. Object::deleteProperty(JSGlobalObject*, uint64_t) also does. A bit unfortunate that the two functions will do the work to make the Identifier string twice, but I suppose it’s an exotic case that we don’t need to optimize carefully. Similarly, we will end up calling jsNumber on the same number repeatedly, which would waste a bit of heap, but again only in exotic cases.

[Saam Barati]

Comment on attachment 407289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407289&action=review

r=me too

>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:874
>>> +    JSValue result = thisObj->get(globalObject, index);
>> 
>> Isn’t the whole point of the previous code that we need the Identifier when at the MAX_ARRAY_INDEX boundary?
> 
> Perhaps, but since Object::get(JSObject*, uint64_t) takes care of creating the identifier when it’s needed, this code will work properly without separate branches for valid array index values and other values. Object::deleteProperty(JSGlobalObject*, uint64_t) also does. A bit unfortunate that the two functions will do the work to make the Identifier string twice, but I suppose it’s an exotic case that we don’t need to optimize carefully. Similarly, we will end up calling jsNumber on the same number repeatedly, which would waste a bit of heap, but again only in exotic cases.

Ah! I forgot I added the uint64_t version of this method. I thought we were implicit casting to the uint32_t version and was worried.

jsNumber on the same number repeatedly is cheap, and doesn't allocate anything in the heap.

[Darin Adler]

Comment on attachment 407289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407289&action=review

>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:874
>>>> +    JSValue result = thisObj->get(globalObject, index);
>>> 
>>> Isn’t the whole point of the previous code that we need the Identifier when at the MAX_ARRAY_INDEX boundary?
>> 
>> Perhaps, but since Object::get(JSObject*, uint64_t) takes care of creating the identifier when it’s needed, this code will work properly without separate branches for valid array index values and other values. Object::deleteProperty(JSGlobalObject*, uint64_t) also does. A bit unfortunate that the two functions will do the work to make the Identifier string twice, but I suppose it’s an exotic case that we don’t need to optimize carefully. Similarly, we will end up calling jsNumber on the same number repeatedly, which would waste a bit of heap, but again only in exotic cases.
> 
> Ah! I forgot I added the uint64_t version of this method. I thought we were implicit casting to the uint32_t version and was worried.
> 
> jsNumber on the same number repeatedly is cheap, and doesn't allocate anything in the heap.

Oh, right, they all get converted to double, and a double always fits in a double. Some values lose precision but they all fit.

When we added the uint64_t version it did create one subtle poor design thing. The 32-bit version has a required precondition that the number is an array index; you must not pass it the value 0xFFFFFFFF. The 64-bit version does not have that precondition, nor do other overloads. Although maybe there is an Identifier one that you must not call with a string that just happens to be a serialized array index. Requiring one number be avoided a strange difference to based only on the passed integer’s width. So I might have introduced a difference in name by renaming the 32-bit integer functions.

[Saam Barati]

(In reply to Darin Adler from comment #6)
> Comment on attachment 407289 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407289&action=review
> 
> >>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:874
> >>>> +    JSValue result = thisObj->get(globalObject, index);
> >>> 
> >>> Isn’t the whole point of the previous code that we need the Identifier when at the MAX_ARRAY_INDEX boundary?
> >> 
> >> Perhaps, but since Object::get(JSObject*, uint64_t) takes care of creating the identifier when it’s needed, this code will work properly without separate branches for valid array index values and other values. Object::deleteProperty(JSGlobalObject*, uint64_t) also does. A bit unfortunate that the two functions will do the work to make the Identifier string twice, but I suppose it’s an exotic case that we don’t need to optimize carefully. Similarly, we will end up calling jsNumber on the same number repeatedly, which would waste a bit of heap, but again only in exotic cases.
> > 
> > Ah! I forgot I added the uint64_t version of this method. I thought we were implicit casting to the uint32_t version and was worried.
> > 
> > jsNumber on the same number repeatedly is cheap, and doesn't allocate anything in the heap.
> 
> Oh, right, they all get converted to double, and a double always fits in a
> double. Some values lose precision but they all fit.
> 
> When we added the uint64_t version it did create one subtle poor design
> thing. The 32-bit version has a required precondition that the number is an
> array index; you must not pass it the value 0xFFFFFFFF. The 64-bit version
> does not have that precondition, nor do other overloads. Although maybe
> there is an Identifier one that you must not call with a string that just
> happens to be a serialized array index. Requiring one number be avoided a
> strange difference to based only on the passed integer’s width. So I might
> have introduced a difference in name by renaming the 32-bit integer
> functions.

We could rename them. Or invent a new type to pass in as a wrapper to the index. Something like ArrayIndex, and it asserts the value is in range, and it's not implicitly constructible.

[Darin Adler]

(In reply to Saam Barati from comment #7)
> We could rename them. Or invent a new type to pass in as a wrapper to the
> index. Something like ArrayIndex, and it asserts the value is in range, and
> it's not implicitly constructible.

We are thinking similarly: I literally considered that option too when writing the comment above, but thought it was too complicated to explain so left it out.

[Saam Barati]

(In reply to Darin Adler from comment #8)
> (In reply to Saam Barati from comment #7)
> > We could rename them. Or invent a new type to pass in as a wrapper to the
> > index. Something like ArrayIndex, and it asserts the value is in range, and
> > it's not implicitly constructible.
> 
> We are thinking similarly: I literally considered that option too when
> writing the comment above, but thought it was too complicated to explain so
> left it out.

I think the right way to do this is to change those helper methods, but also to change the method table methods of:
- putByIndex
- deletePropertyByIndex
- getOwnPropertySlotByIndex

[Saam Barati]

(In reply to Saam Barati from comment #9)
> (In reply to Darin Adler from comment #8)
> > (In reply to Saam Barati from comment #7)
> > > We could rename them. Or invent a new type to pass in as a wrapper to the
> > > index. Something like ArrayIndex, and it asserts the value is in range, and
> > > it's not implicitly constructible.
> > 
> > We are thinking similarly: I literally considered that option too when
> > writing the comment above, but thought it was too complicated to explain so
> > left it out.
> 
> I think the right way to do this is to change those helper methods, but also
> to change the method table methods of:
> - putByIndex
> - deletePropertyByIndex
> - getOwnPropertySlotByIndex

I might actually be wrong. Reading some more code.

[Saam Barati]

(In reply to Darin Adler from comment #6)
> Comment on attachment 407289 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407289&action=review
> 
> >>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:874
> >>>> +    JSValue result = thisObj->get(globalObject, index);
> >>> 
> >>> Isn’t the whole point of the previous code that we need the Identifier when at the MAX_ARRAY_INDEX boundary?
> >> 
> >> Perhaps, but since Object::get(JSObject*, uint64_t) takes care of creating the identifier when it’s needed, this code will work properly without separate branches for valid array index values and other values. Object::deleteProperty(JSGlobalObject*, uint64_t) also does. A bit unfortunate that the two functions will do the work to make the Identifier string twice, but I suppose it’s an exotic case that we don’t need to optimize carefully. Similarly, we will end up calling jsNumber on the same number repeatedly, which would waste a bit of heap, but again only in exotic cases.
> > 
> > Ah! I forgot I added the uint64_t version of this method. I thought we were implicit casting to the uint32_t version and was worried.
> > 
> > jsNumber on the same number repeatedly is cheap, and doesn't allocate anything in the heap.
> 
> Oh, right, they all get converted to double, and a double always fits in a
> double. Some values lose precision but they all fit.
> 
> When we added the uint64_t version it did create one subtle poor design
> thing. The 32-bit version has a required precondition that the number is an
> array index; you must not pass it the value 0xFFFFFFFF. The 64-bit version
> does not have that precondition, nor do other overloads. Although maybe
> there is an Identifier one that you must not call with a string that just
> happens to be a serialized array index. Requiring one number be avoided a
> strange difference to based only on the passed integer’s width. So I might
> have introduced a difference in name by renaming the 32-bit integer
> functions.

After reading some code, it doesn't appear that there are restrictions on the range of the unsigned passed into these methods. For example, 0xFFFFFFFF should be handled just fine:

```
    ALWAYS_INLINE bool putByIndexInline(JSGlobalObject* globalObject, unsigned propertyName, JSValue value, bool shouldThrow)
    {
        VM& vm = getVM(globalObject);
        if (canSetIndexQuickly(propertyName, value)) {
            setIndexQuickly(vm, propertyName, value);
            return true;
        }
        return methodTable(vm)->putByIndex(this, globalObject, propertyName, value, shouldThrow);
    }
```

canSetIndexQuickly for 0xFFFFFFFF should return false. This will lead us to taking the putByIndex path. Let's look at one example implementation:

```
bool JSObject::putByIndex(JSCell* cell, JSGlobalObject* globalObject, unsigned propertyName, JSValue value, bool shouldThrow)
{
    VM& vm = globalObject->vm();
    JSObject* thisObject = jsCast<JSObject*>(cell);

    if (propertyName > MAX_ARRAY_INDEX) {
        PutPropertySlot slot(cell, shouldThrow);
        return thisObject->methodTable(vm)->put(thisObject, globalObject, Identifier::from(vm, propertyName), value, slot);
    }
...
...
```

So it's expected that the various method table methods I listed properly handle all ranges of values passed into the various byIndex MethodTable methods.

[Darin Adler]

(In reply to Saam Barati from comment #11)
> it doesn't appear that there are restrictions on
> the range of the unsigned passed into these methods

OK, glad to hear it. Guess I remembered wrong. I’m pretty sure I’m the one who created these separate overloads so that integers could pass through the runtime without being converted to and from Identifier strings, but I must have forgotten some of the details over the years.

[Saam Barati]

(In reply to Darin Adler from comment #12)
> (In reply to Saam Barati from comment #11)
> > it doesn't appear that there are restrictions on
> > the range of the unsigned passed into these methods
> 
> OK, glad to hear it. Guess I remembered wrong. I’m pretty sure I’m the one
> who created these separate overloads so that integers could pass through the
> runtime without being converted to and from Identifier strings, but I must
> have forgotten some of the details over the years.

Yeah, the code itself is a bit subtle, and I have to re-read the code to convince myself of its correctness. It took me a second, just now, to realize that our "isIndex" function and MAX_ARRAY_INDEX are indeed in sync. One nit I have with the code base is to rewrite isIndex in terms of MAX_ARRAY_INDEX, since we have code that relies on > MAX_ARRAY_INDEX having "isIndex" return false. Currently, we just hard code numbers in two different places.

[Darin Adler]

(In reply to Saam Barati from comment #13)
> One nit I have with the code base is to rewrite isIndex in terms of
> MAX_ARRAY_INDEX, since we have code that relies on > MAX_ARRAY_INDEX having
> "isIndex" return false. Currently, we just hard code numbers in two
> different places.

Compared to MAX_ARRAY_INDEX, isIndex is new. Yusuke added it in 2015:

https://trac.webkit.org/changeset/182452

MAX_ARRAY_INDEX is many years older and used many more places.

[Saam Barati]

(In reply to Darin Adler from comment #14)
> (In reply to Saam Barati from comment #13)
> > One nit I have with the code base is to rewrite isIndex in terms of
> > MAX_ARRAY_INDEX, since we have code that relies on > MAX_ARRAY_INDEX having
> > "isIndex" return false. Currently, we just hard code numbers in two
> > different places.
> 
> Compared to MAX_ARRAY_INDEX, isIndex is new. Yusuke added it in 2015:
> 
> https://trac.webkit.org/changeset/182452
> 
> MAX_ARRAY_INDEX is many years older and used many more places.

Will do a small cleanup in:
https://bugs.webkit.org/show_bug.cgi?id=215872

[Alexey Shvayka]

Committed r266215: <https://trac.webkit.org/changeset/266215>

[Radar WebKit Bug Importer]

<rdar://problem/67843783>