Bug 221245

Summary: [JSC] Atomics should support BigInt64Array / BigUint64Array
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch keith_miller: review+

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>