Bug 150216 – Fix some issues with TypedArrays

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Fix some issues with TypedArrays

[Keith Miller]

Created attachment 263284 [details]
Patch

[Keith Miller]

Created attachment 263291 [details]
Patch

[Michael Saboff]

Comment on attachment 263291 [details]
Patch

r=me

[Keith Miller]

Created attachment 263300 [details]
Benchmark Results

Benchmark results since adding iterators during construction in the DFG may have slowed things down (they didn't). Note: "string-unpack-code" does not use TypedArrays and subsequent reruns showed no performance difference.

[Keith Miller]

Committed r191190: <http://trac.webkit.org/changeset/191190>

[Geoffrey Garen]

Comment on attachment 263291 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:609
> +    return bitwise_cast<char*>(constructGenericTypedArrayViewWithFirstArgument<JSInt8Array>(exec, structure, encodedValue));

reinterpret_cast<char*> is better here because it is slightly more type-safe.

Cast to char* is allowed despite strict aliasing rules because char* is considered to alias everything, so cast to char* is, from the aliasing point of view, just a case to a more generic type.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:118
> +template<typename ViewClass, bool checkForOtherArguments = false>
> +static JSObject* constructGenericTypedArrayViewWithFirstArgument(ExecState* exec, Structure* structure, EncodedJSValue firstArgument)

Boolean parameters read poorly at the call site. Nobody knows what "create(true, false, true)" means. For future reference, WebKit style suggests limiting or eliminating boolean parameters.

In this case, I think it is better to eliminate the checkForOtherArguments parameter. It tightly couples this function to a single caller (the constructor callback), which is usually a programming anti-pattern.

How about this:

template<typename ViewClass> static JSObject* constructGenericTypedArrayView(ExecState* exec, EncodedJSValue argument, unsigned offset, const WTF::Optional<unsigned>& length) { ... }

All of our callers know their offset (usually it's zero, while the constructor needs to check its arguments). Most of our caller don't supply a length while the constructor sometimes does, so we have Optional for that.

This pushes the calculation and conversion of constructor input arguments back up into the constructor function, which is where it belongs.

Technically, this adds one branch to some DFG typed array allocations. I don't think that will matter.

[Keith Miller]

Reverted r191190 for reason:

Patch needs some design changes.

Committed r191193: <http://trac.webkit.org/changeset/191193>

[Keith Miller]

Created attachment 263324 [details]
Patch

[Geoffrey Garen]

Comment on attachment 263324 [details]
Patch

r=me

[Keith Miller]

Committed r191212: <http://trac.webkit.org/changeset/191212>