Bug 221245 - [JSC] Atomics should support BigInt64Array / BigUint64Array
Summary: [JSC] Atomics should support BigInt64Array / BigUint64Array
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-01 22:17 PST by Yusuke Suzuki
Modified: 2021-02-03 15:08 PST (History)
7 users (show)

See Also:


Attachments
Patch (42.37 KB, patch)
2021-02-01 22:20 PST, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-02-01 22:17:05 PST
...
Comment 1 Yusuke Suzuki 2021-02-01 22:20:41 PST
Created attachment 418962 [details]
Patch
Comment 2 Yusuke Suzuki 2021-02-02 12:18:11 PST
Comment on attachment 418962 [details]
Patch

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

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:354
> +    RELEASE_AND_RETURN(scope, JSValue::encode(value));

We need to return this |value|, not the value after `toNativeFromValue`'s enforcement is done.
Comment 3 Keith Miller 2021-02-03 12:30:21 PST
Comment on attachment 418962 [details]
Patch

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

r=me but I have some comments.

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:172
> +EncodedJSValue atomicReadModifyWrite(JSGlobalObject* globalObject, VM& vm, const JSValue* args, const Func& func)

Why change the argument order here? We typically use VM as the first argument.

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:338
> +    typename Adaptor::Type extraArgs[1];

Why is this an array? Just so you don't have to & in the StoreFunc? If so I don't think that's worth it...

> Source/JavaScriptCore/runtime/AtomicsObject.cpp:363
> +    JSArrayBufferView* typedArrayView = validateIntegerTypedArray<TypedArrayOperationMode::Read>(globalObject, base);

Can we change the enum values? The current ones don't make any sense... why is atomicStore checking Read??

I'd name them ReadWrite and Wait(able), but I'm flexible.
Comment 4 Mark Lam 2021-02-03 12:32:41 PST
Comment on attachment 418962 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        2. Extend Atomics.isLockFree to accept 8.

I suggest adding a bit more detail: "to also accept a size of 8".
Comment 5 Yusuke Suzuki 2021-02-03 15:04:56 PST
Comment on attachment 418962 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:12
>> +        2. Extend Atomics.isLockFree to accept 8.
> 
> I suggest adding a bit more detail: "to also accept a size of 8".

Added

>> Source/JavaScriptCore/runtime/AtomicsObject.cpp:172
>> +EncodedJSValue atomicReadModifyWrite(JSGlobalObject* globalObject, VM& vm, const JSValue* args, const Func& func)
> 
> Why change the argument order here? We typically use VM as the first argument.

We put JSGlobalObject* first when the function can throw an error.

>> Source/JavaScriptCore/runtime/AtomicsObject.cpp:338
>> +    typename Adaptor::Type extraArgs[1];
> 
> Why is this an array? Just so you don't have to & in the StoreFunc? If so I don't think that's worth it...

This is for StoreFunc's signature. But since StoreFunc is only sued here, we can remove StoreFunc and just use atomicStoreFullyFenced directly here. Changed

>> Source/JavaScriptCore/runtime/AtomicsObject.cpp:363
>> +    JSArrayBufferView* typedArrayView = validateIntegerTypedArray<TypedArrayOperationMode::Read>(globalObject, base);
> 
> Can we change the enum values? The current ones don't make any sense... why is atomicStore checking Read??
> 
> I'd name them ReadWrite and Wait(able), but I'm flexible.

Oops, yeah. Fixed to ReadWrite / Wait.
Comment 6 Yusuke Suzuki 2021-02-03 15:07:55 PST
Committed r272341: <https://trac.webkit.org/changeset/272341>
Comment 7 Radar WebKit Bug Importer 2021-02-03 15:08:14 PST
<rdar://problem/73951609>