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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 13 19:44:40 PDT 2019

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

Attachment 376228: Patch


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

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.

More information about the webkit-reviews mailing list