[JSC] add support for Uint8Array.prototype.toBase64 and Uint8Array.prototype.toHex
#29039
Conversation
dcrousso
added
the
JavaScriptCore
|
EWS run on previous version of this PR (hash 5dd43af) |
|
will add some tests for this in the next few days :) |
|
EWS run on previous version of this PR (hash 6d866d7) |
| JSValue thisValue = callFrame->thisValue(); | ||
| if (!thisValue.isObject()) | ||
| return throwVMTypeError(globalObject, scope, "Receiver should be a typed array view but was not an object"_s); | ||
|
|
||
| JSObject* thisObject = jsCast<JSObject*>(thisValue); | ||
| if (thisObject->type() != Uint8ArrayType) | ||
| return throwVMTypeError(globalObject, scope, "Uint8Array.prototype.toBase64 requires that |this| be a Uint8Array"_s); | ||
|
|
||
| JSUint8Array* uint8Array = jsCast<JSUint8Array*>(thisObject); |
There was a problem hiding this comment.
Let's use jsDynamicCast<JSUint8Array*>(thisValue) and check whether it is non null, like
JSUint8Array* uint8Array = jsDynamicCast<JSUint8Array*>(callFrame->thisValue());
if (UNLIKELY(!uint8Array))
return throwVMTypeError(globalObject, scope, "Uint8Array.prototype.toBase64 requires that |this| be a Uint8Array"_s);
There was a problem hiding this comment.
nice! ive always wondered when to use jsDynamicCast vs jsCast. is there a general rule i could follow to answer that?
There was a problem hiding this comment.
If you're going to check if a JSValue is some type we typically use jsDynamicCast. If you know the JSValue is actually the type already we use jsCast. There are other mechanisms but those are mostly for legacy code.
| JSValue thisValue = callFrame->thisValue(); | ||
| if (!thisValue.isObject()) | ||
| return throwVMTypeError(globalObject, scope, "Receiver should be a typed array view but was not an object"_s); | ||
|
|
||
| JSObject* thisObject = jsCast<JSObject*>(thisValue); | ||
| if (thisObject->type() != Uint8ArrayType) | ||
| return throwVMTypeError(globalObject, scope, "Uint8Array.prototype.toHex requires that |this| be a Uint8Array"_s); |
|
|
||
| JSObject* optionsObject = jsCast<JSObject*>(optionsValue); | ||
|
|
||
| JSValue alphabetValue = optionsObject->get(globalObject, Identifier::fromString(vm, "alphabet"_s)); |
There was a problem hiding this comment.
Let's define alphabet in CommonIdentifier
| const uint8_t* data = uint8Array->typedVector(); | ||
| size_t offset = uint8Array->byteOffset(); | ||
| size_t length = uint8Array->length(); | ||
| RELEASE_AND_RETURN(scope, JSValue::encode(jsString(vm, base64EncodeToString({ data + offset, length }, mode)))); |
There was a problem hiding this comment.
This is not correct. typedVector is already offseted vector pointer. byteOffset is indicating the offset in the original buffer. Not in the typedVector. Can you add a test for that?
There was a problem hiding this comment.
oh! interesting. i guess i didnt look at this closely enough as i can clearly see this in JSArrayBufferView::ConstructionContext 😅
yes i will add some tests specifically for this scenario
| const uint8_t* data = uint8Array->typedVector(); | ||
| size_t offset = uint8Array->byteOffset(); | ||
| size_t length = uint8Array->length(); | ||
| for (size_t i = offset; i < length; ++i) | ||
| builder.append(toStringWithRadix(data[i], 16)); | ||
| RELEASE_AND_RETURN(scope, JSValue::encode(jsString(vm, builder.toString()))); |
|
EWS run on previous version of this PR (hash 8e61aa2) |
| const uint8_t* data = uint8Array->typedVector(); | ||
| size_t length = uint8Array->length(); | ||
| for (size_t i = 0; i < length; ++i) | ||
| builder.append(toStringWithRadix(data[i], 16)); |
There was a problem hiding this comment.
This is allocating String per byte, which is too costly.
There was a problem hiding this comment.
Incase it's helpful, in Bun we implemented the Node.js API Buffer.prototype.toString("hex"):
It uses SIMD and is around 10x faster than Node.js' scalar implementation
| validateTypedArray(globalObject, uint8Array); | ||
|
|
||
| StringBuilder builder; | ||
| builder.reserveCapacity(uint8Array->length()); |
There was a problem hiding this comment.
Doesn't one byte become 2 characters? (255 => ff)
| JSValue alphabetValue = optionsObject->get(globalObject, vm.propertyNames->alphabet); | ||
| RETURN_IF_EXCEPTION(scope, { }); | ||
| if (!alphabetValue.isUndefined()) { | ||
| if (!alphabetValue.isString()) | ||
| return throwVMTypeError(globalObject, scope, "Uint8Array.prototype.toBase64 requires that alphabet be \"base64\" or \"base64url\""_s); | ||
|
|
||
| String alphabet = alphabetValue.toWTFString(globalObject); | ||
| if (alphabet == "base64url"_s) | ||
| mode = Base64EncodeMode::URL; | ||
| else if (alphabet != "base64"_s) | ||
| return throwVMTypeError(globalObject, scope, "Uint8Array.prototype.toBase64 requires that alphabet be \"base64\" or \"base64url\""_s); | ||
| } |
There was a problem hiding this comment.
These things have side-effect since it is invoking toString, [[Get]], so they can detach the typed array.
Doesn't the spec checks the condition after that?
|
EWS run on previous version of this PR (hash 8393ed6) |
|
EWS run on previous version of this PR (hash 92ee92e) |
| } | ||
|
|
||
| shouldBe((new Uint8Array([])).toBase64({alphabet: "base64url"}), ""); | ||
| shouldBe((new Uint8Array([0])).toBase64({alphabet: "base64url"}), "AA"); |
There was a problem hiding this comment.
i just realized these are all missing padding =, which im fixing as a prerequisite in #29530 (which should also help if/when omitPadding is approved in TC39 plenary later this month)
|
EWS run on current version of this PR (hash e28c233) |
….prototype.toHex` https://bugs.webkit.org/show_bug.cgi?id=274635 Reviewed by Yusuke Suzuki. These methods will make it easier for developers to encode typed character/byte data. Note that there are other methods in the related proposal, but for ease of reviewing they will be implemented separately. Spec: <https://tc39.es/proposal-arraybuffer-base64/spec/> Proposal: <https://github.com/tc39/proposal-arraybuffer-base64> * Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.h: * Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeInlines.h: (JSC::JSGenericTypedArrayViewPrototype<ViewClass>::finishCreation): * Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.cpp: Added. (JSC::uint8ArrayPrototypeToBase64): (JSC::uint8ArrayPrototypeToHex): * Source/JavaScriptCore/runtime/CommonIdentifiers.h: Create builtin identifiers for `alphabet` and `omitPadding`. * Source/JavaScriptCore/runtime/OptionsList.h: Add a `useUint8ArrayBase64Methods` feature flag for these new methods (and the others that are part of the proposal that haven't been implemented yet). * Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj: * Source/JavaScriptCore/Sources.txt: * JSTests/stress/uint8array-toBase64.js: Added. * JSTests/stress/uint8array-toHex.js: Added. Canonical link: https://commits.webkit.org/280654@main
webkit-commit-queue
force-pushed
the
eng/274635
branch
from
e28c233 to
2596e19
Compare
|
Committed 280654@main (2596e19): https://commits.webkit.org/280654@main Reviewed commits have been landed. Closing PR #29039 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
2596e19
e28c233
🧪 wpe-wk2🧪 ios-wk2🧪 api-ios🧪 mac-AS-debug-wk2🧪 gtk-wk2🧪 api-gtk🛠 tv🛠 watch