[webkit-reviews] review denied: [Bug 200181] Identify memcpy loops in b3 : [Attachment 376228] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 200181: Identify memcpy loops in b3
https://bugs.webkit.org/show_bug.cgi?id=200181

Attachment 376228: Patch

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




--- Comment #38 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 376228
  --> https://bugs.webkit.org/attachment.cgi?id=376228
Patch

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

> Source/JavaScriptCore/b3/B3ReduceLoopStrength.cpp:49
> + * If we would use system memcpy, then it is possibly that we would get a
byte copy loop,

possibly => possible

> Source/JavaScriptCore/runtime/JSObject.h:198
>      ALWAYS_INLINE bool putByIndexInline(ExecState* exec, unsigned
propertyName, JSValue value, bool shouldThrow)

why not pass a VM through to here?

> Source/JavaScriptCore/runtime/JSObject.h:360
> +	   if (canSetIndexQuicklyForTypedArray(vm, i))
> +	       return true;

you should put this inside the switch below for the corresponding indexing
type. IIRC, indexing type for typed arrays is blank

> Source/JavaScriptCore/runtime/JSObject.h:386
> +    bool canSetIndexQuicklyForTypedArray(VM&, unsigned);
> +    void setIndexQuicklyForTypedArray(VM&, unsigned, JSValue);

why not get too?

> Source/JavaScriptCore/runtime/JSObjectInlines.h:409
> +	   if (auto* typedArray = jsCast<JS ## name ## Array *>(this))\

this is wrong. You are looking for jsDynamicCast, which might return null. But
why do you need this type check anyways? Is asking class info not enough?

Also, instead of loading the class info, I think you can just check the JSType
on the object itself. I think that should be faster since it's not a dependent
load. You also won't have to thread through VM.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:426
> +	   ASSERT(typedArray);\

this assert is bogus. See above as well for using JSType.