Bug 179899 – Allow for more efficient use of GenericTypedArrayView

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

A few fixes to GenericTypedArrayView to make callers faster:
* add setNative(unsigned index, typename Adaptor::Type value) to avoid uint_8 -> double -> uint_8 conversions
* add getRange() to read a range of bytes, akin to setRange
* avoid getRange and setRange using the virtual byteLength

[Simon Fraser (smfr)]

Created attachment 327376 [details]
Patch

[EWS Watchlist]

Attachment 327376 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:178:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:179:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:180:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:184:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:186:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:193:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/GenericTypedArrayView.h:94:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 7 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Created attachment 327378 [details]
Patch

[EWS Watchlist]

Attachment 327378 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:178:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:193:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/JavaScriptCore/runtime/GenericTypedArrayView.h:94:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

(In reply to Build Bot from comment #4)
> Attachment 327378 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/runtime/ArrayBufferView.h:178:  Please replace
> ASSERT_WITH_SECURITY_IMPLICATION() with
> RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]

Nope; the assertion runs a virtual function.

[WebKit Commit Bot]

Comment on attachment 327378 [details]
Patch

Clearing flags on attachment: 327378

Committed r225084: <https://trac.webkit.org/changeset/225084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35658463>

[Darin Adler]

Comment on attachment 327378 [details]
Patch

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

> Source/JavaScriptCore/runtime/ArrayBufferView.h:122
> +    inline bool setRangeImpl(const char* data, size_t dataByteLength, unsigned byteOffset, unsigned bufferByteLength);
> +    inline bool getRangeImpl(char* destination, size_t dataByteLength, unsigned byteOffset, unsigned bufferByteLength);

The "inline" on all these functions are not needed.

Should these be void* instead of char*? Or maybe uint8_t*?

[Simon Fraser (smfr)]

Followup in https://trac.webkit.org/changeset/225106/webkit

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #9)
> Comment on attachment 327378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327378&action=review
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:122
> > +    inline bool setRangeImpl(const char* data, size_t dataByteLength, unsigned byteOffset, unsigned bufferByteLength);
> > +    inline bool getRangeImpl(char* destination, size_t dataByteLength, unsigned byteOffset, unsigned bufferByteLength);
> 
> The "inline" on all these functions are not needed.

Without them I get duplicate symbol errors at link time.

> Should these be void* instead of char*? Or maybe uint8_t*?

void*, yes. Will fix in a followup.

[Simon Fraser (smfr)]

More cleanup in aisle, er bug 179966

[Darin Adler]

Comment on attachment 327378 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ArrayBufferView.h:122
>>> +    inline bool getRangeImpl(char* destination, size_t dataByteLength, unsigned byteOffset, unsigned bufferByteLength);
>> 
>> The "inline" on all these functions are not needed.
>> 
>> Should these be void* instead of char*? Or maybe uint8_t*?
> 
> Without them I get duplicate symbol errors at link time.

What I meant is that the inline should be on the function definitions, not here in the class. Not removing them entirely.

But maybe I am wrong about that. Maybe we should be moving toward doing it this way consistently instead, moving the "inline" keywords from the function definitions to the function declarations in the class above. There is at least one benefit to that, I think. The compiler can produce a warning or error if you call the function and it doesn’t have the function definition in the compilation unit, moving what would be a linker error to compile time.