Summary: | [JSC] Atomics should support BigInt64Array / BigUint64Array | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
Component: | JavaScriptCore | Assignee: | 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
Yusuke Suzuki
2021-02-01 22:17:05 PST
Created attachment 418962 [details]
Patch
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 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 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 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. Committed r272341: <https://trac.webkit.org/changeset/272341> |