[webkit-reviews] review granted: [Bug 211279] Merge putLength() into setLength() : [Attachment 407289] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 211279: Merge putLength() into setLength()
https://bugs.webkit.org/show_bug.cgi?id=211279

Attachment 407289: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 407289
  --> https://bugs.webkit.org/attachment.cgi?id=407289
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.