Skip to content

Conversation

@Constellation
Copy link
Member

@Constellation Constellation commented Jul 17, 2024

2a040cb

[JSC] Implement fromHex and setFromHex in SIMD
https://bugs.webkit.org/show_bug.cgi?id=276705
rdar://131903377

Reviewed by Keith Miller and Justin Michaud.

This patch implements fromHex / setFromHex in SIMD. The new implementation is roughly 17x faster.
We use http://0x80.pl/notesen/2022-01-17-validating-hex-parse.html 's algorithm 3 approach to decode hex in SIMD with validation.
For 16bit string, we load high and low bytes separately, checking high bytes are all zero, and decoding low bytes as the same to
8bit string with validation.

                               ToT                     Patched

    from-hex             46.8263+-0.1123     ^      2.7441+-0.0149        ^ definitely 17.0641x faster

* JSTests/microbenchmarks/from-hex.js: Added.
(test):
* JSTests/stress/uint8array-fromHex.js:
(shouldBeArray.Uint8Array.fromHex):
(shouldBeArray):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.cpp:
(JSC::decodeHexImpl):
(JSC::decodeHex):
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/281053@main

c13071f

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2 🧪 wincairo-tests
✅ 🧪 webkitperl 🧪 ios-wk2 🧪 api-mac 🧪 api-wpe
🧪 ios-wk2-wpt 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc 🧪 api-ios 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision 🧪 mac-AS-debug-wk2 🧪 gtk-wk2
✅ 🛠 vision-sim 🧪 mac-wk2-stress 🧪 api-gtk
🧪 vision-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Constellation Constellation self-assigned this Jul 17, 2024
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jul 17, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 17, 2024
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Jul 17, 2024
@Constellation Constellation force-pushed the eng/JSC-Implement-fromHex-and-setFromHex-in-SIMD branch from 13e7211 to 8253efb Compare July 17, 2024 07:54
@Constellation Constellation force-pushed the eng/JSC-Implement-fromHex-and-setFromHex-in-SIMD branch from 8253efb to 8457448 Compare July 17, 2024 07:56
@Constellation Constellation force-pushed the eng/JSC-Implement-fromHex-and-setFromHex-in-SIMD branch from 8457448 to 899e62d Compare July 17, 2024 08:40
@Constellation Constellation marked this pull request as ready for review July 17, 2024 08:45
@Constellation Constellation requested a review from a team as a code owner July 17, 2024 08:45
@Constellation Constellation force-pushed the eng/JSC-Implement-fromHex-and-setFromHex-in-SIMD branch from 899e62d to 2a9cb1e Compare July 17, 2024 09:00

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the memory will be fully written, couldn't uninitialized memory be used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@justinmichaud justinmichaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rs=me

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with comments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think this would be easier to read if this lambda wasn't defined inside the if condition. So something like:

if (span.size() >= stride) {
    auto doStridedDecode = [&]() ALWAYS_INLINE_LAMBDA { ... };
    if (!doStridedDecode())
        return WTF::notFound;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member Author

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Constellation Constellation force-pushed the eng/JSC-Implement-fromHex-and-setFromHex-in-SIMD branch from 2a9cb1e to c13071f Compare July 17, 2024 15:32
@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jul 17, 2024
https://bugs.webkit.org/show_bug.cgi?id=276705
rdar://131903377

Reviewed by Keith Miller and Justin Michaud.

This patch implements fromHex / setFromHex in SIMD. The new implementation is roughly 17x faster.
We use http://0x80.pl/notesen/2022-01-17-validating-hex-parse.html 's algorithm 3 approach to decode hex in SIMD with validation.
For 16bit string, we load high and low bytes separately, checking high bytes are all zero, and decoding low bytes as the same to
8bit string with validation.

                               ToT                     Patched

    from-hex             46.8263+-0.1123     ^      2.7441+-0.0149        ^ definitely 17.0641x faster

* JSTests/microbenchmarks/from-hex.js: Added.
(test):
* JSTests/stress/uint8array-fromHex.js:
(shouldBeArray.Uint8Array.fromHex):
(shouldBeArray):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.cpp:
(JSC::decodeHexImpl):
(JSC::decodeHex):
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/281053@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Implement-fromHex-and-setFromHex-in-SIMD branch from c13071f to 2a040cb Compare July 17, 2024 15:35
@webkit-commit-queue
Copy link
Collaborator

Committed 281053@main (2a040cb): https://commits.webkit.org/281053@main

Reviewed commits have been landed. Closing PR #30897 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 2a040cb into WebKit:main Jul 17, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jul 17, 2024
@Constellation Constellation deleted the eng/JSC-Implement-fromHex-and-setFromHex-in-SIMD branch July 17, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants