[webkit-reviews] review granted: [Bug 180768] JSObjects should have a mask for loading indexed properties : [Attachment 329277] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 180768: JSObjects should have a mask for loading indexed properties
https://bugs.webkit.org/show_bug.cgi?id=180768

Attachment 329277: Patch

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




--- Comment #10 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 329277
  --> https://bugs.webkit.org/attachment.cgi?id=329277
Patch

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

r=me with fixes.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1065
> +	       // If we were to have any indexed properties then we would need
update the indexing mask on the base object.

did you mean "would need to update"?  I also suggest putting a comma after
"indexed properties" before "then".

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7701
> +	   // X86 only has 6 GP registers, which is not enough for the fast
case here. At least without custom code, which is not currently worth the extra
code mantainence.

/mantainence/maintenance/.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1377
> +    // sizeGPR may be either scratch register. If for whatever reason the
butterfly is going to change vector length this function does NOT
> +    // update the indexing mask.

This comment seems out of place.  There's no use of sizeGPR in the function
below.

> Source/JavaScriptCore/runtime/Butterfly.h:135
> +    // If vectorLength() is zero clz is undefined.
> +    uint32_t computeIndexingMask() { return
computeIndexingMaskForVectorLength(vectorLength()); } // vectorLength() ?
~((1ll << 63) >> std::clz(static_cast<uint64_t>(vectorLength()) *
sizeof(EncodedJSValue))) : 0; }

The comment at line 134 says that clz is undefined for a zero vectorLength, but
then you go ahead and just call computeIndexingMaskForVectorLength() which uses
std::clz() without a zero check first.
The comment after the function at line 135 doesn't seem to describe what this
function does.	It says that the return value is scaled by
sizeof(EncodedJSValue).  I think that is wrong.

Please fix.

> Source/JavaScriptCore/runtime/JSObject.cpp:2611
> +    auto oldButterfly = butterfly;
> +    UNUSED_PARAM(oldButterfly);

What is this for?  Please remove if not needed.