gh-Cyan4973/xxHash
Watch
1
0
Fork
You've already forked xxHash
 0
Code Issues 7 Pull requests 3 Releases 26 Wiki Activity

Add WebAssembly SIMD128 implementation and Node.JS support #825

Merged
easyaspi314 merged 8 commits from wasm128 into dev 2023-07-10 00:21:00 -05:00
Conversation 25 Commits 8 Files changed 7 +202 -51
easyaspi314 commented 2023-03-14 11:25:20 -05:00 (Migrated from github.com)
  • Support WebAssembly SIMD128 via the NEON path
    • The Emscripten SDK includes the SIMDeverywhere polyfill for <arm_neon.h>
    • Since SIMD128 seems to be designed after a subset of NEON, the double width NEON path maps 1:1 and gets full speed with few modifications
      • Note: XXH3_NEON_LANES != 8 IS BAD since SIMDe scalarizes half vectors
    • On v8, about 2x faster than scalar on AArch64, 3-4x faster on x86_64
    • Firefox seems to be slightly faster on x86, slightly slower on aarch64
  • Adds a few fixes for compiling with Node.JS (make NODE_JS=1)
    • Requires linking nodefs and noderawfs to access local files
    • Hook into node.js directly using inline JavaScript to access tty.isatty() from Node instead of the broken isatty() in libc (which always returns 1 for stdin/stdout/stderr)
  • Add wasm/asmjs to the welcome message
  • Remove emcc output files on make clean
  • Add basic CI tests for both WASM and asm.js (the latter of which is painfully slow lol)
  • Also since I needed to modify XXH3_scrambleAcc_neon, I found a stupidly obvious optimization to standard NEON.

Due to the fact that WebAssembly is JIT-compiled and that JavaScript timer precision is limited due to Spectre, the results may vary.

NEON gets almost native speed on v8, but x86_64 does not mainly due to the instruction sets not exactly matching up. Firefox's aarch64 JIT seems to be considerably slower when it comes to WASM.

xxhsum -b on an AArch64 Google Tensor G1 (Cortex-X1 @ 2.80 GHz):

Browser/engine Scalar XXH3_64b Scalar XXH128 WASM128 XXH3_64B WASM128 XXH128
Node 18.13 / v8 10.2.154 (termux) 8421.7 MB/s 8161.7 MB/s 16485.4 MB/s 16355.0 MB/s
Firefox Nightly for Android 112.0a1 7850.5 MB/s 7870.8 MB/s 6148.4 MB/s 5861.7 MB/s

x86_64 (with AVX2) Core i7-8650U @ 2.11 GHz (turbo 4.20 GHz):

Browser/engine Scalar XXH3_64b Scalar XXH128 WASM128 XXH3_64B WASM128 XXH128
Node 18.14 / v8 10.2.154 (mingw64) 3605.8 MB/s 3473.8 MB/s 9658.2 MB/s 11535.5 MB/s
Firefox for Windows 110.0.1 6406.9 MB/s 6004.3 MB/s 9249.1 MB/s 9288.8 MB/s

(Not sure why XXH128 is that much faster on x86 but not ARM. Maybe code alignment memes?).

- Support WebAssembly SIMD128 via the NEON path - The Emscripten SDK includes the SIMDeverywhere polyfill for `<arm_neon.h>` - Since SIMD128 seems to be designed after a subset of NEON, the double width NEON path maps 1:1 and gets full speed with few modifications - Note: `XXH3_NEON_LANES != 8` IS BAD since SIMDe scalarizes half vectors - On v8, about 2x faster than scalar on AArch64, 3-4x faster on x86_64 - Firefox seems to be slightly faster on x86, slightly slower on aarch64 - Adds a few fixes for compiling with Node.JS (`make NODE_JS=1`) - Requires linking `nodefs` and `noderawfs` to access local files - Hook into node.js directly using inline JavaScript to access `tty.isatty()` from Node instead of the broken `isatty()` in libc (which always returns `1` for stdin/stdout/stderr) - Add `wasm/asmjs` to the welcome message - Remove `emcc` output files on `make clean` - Add basic CI tests for both WASM and asm.js (the latter of which is painfully slow lol) - Also since I needed to modify XXH3_scrambleAcc_neon, I found a stupidly obvious optimization to standard NEON. Due to the fact that WebAssembly is JIT-compiled and that JavaScript timer precision is limited due to Spectre, the results may vary. NEON gets almost native speed on v8, but x86_64 does not mainly due to the instruction sets not exactly matching up. Firefox's aarch64 JIT seems to be considerably slower when it comes to WASM. `xxhsum -b` on an AArch64 Google Tensor G1 (Cortex-X1 @ 2.80 GHz): | Browser/engine | Scalar XXH3_64b | Scalar XXH128 | WASM128 XXH3_64B | WASM128 XXH128 | |:--|:--|:--|:--|:--| | Node 18.13 / v8 10.2.154 (termux) | 8421.7 MB/s | 8161.7 MB/s | 16485.4 MB/s | 16355.0 MB/s | | Firefox Nightly for Android 112.0a1 | 7850.5 MB/s | 7870.8 MB/s | 6148.4 MB/s | 5861.7 MB/s | x86_64 (with AVX2) Core i7-8650U @ 2.11 GHz (turbo 4.20 GHz): | Browser/engine | Scalar XXH3_64b | Scalar XXH128 | WASM128 XXH3_64B | WASM128 XXH128 | |:--|:--|:--|:--|:--| | Node 18.14 / v8 10.2.154 (mingw64) | 3605.8 MB/s | 3473.8 MB/s | 9658.2 MB/s | 11535.5 MB/s | | Firefox for Windows 110.0.1 | 6406.9 MB/s | 6004.3 MB/s | 9249.1 MB/s | 9288.8 MB/s | (Not sure why XXH128 is that much faster on x86 but not ARM. Maybe code alignment memes?).
👍 2
Cyan4973 commented 2023-03-14 11:41:36 -05:00 (Migrated from github.com)

The added source code is pretty clean, this is easy to review.

It seems you want to test it a little longer ?
In which case, could you make this PR "draft", so I don't wonder if it needs to be merged now or later ?

The added source code is pretty clean, this is easy to review. It seems you want to test it a little longer ? In which case, could you make this PR "draft", so I don't wonder if it needs to be merged now or later ?
easyaspi314 commented 2023-03-14 16:25:42 -05:00 (Migrated from github.com)

So apparently marking it as a draft cancels CI tests...

So apparently marking it as a draft cancels CI tests...
👀 1
easyaspi314 commented 2023-03-14 17:04:47 -05:00 (Migrated from github.com)

Ok I think I figured everything out. I want to add various versions of the Emscripten SDK to the matrix but that isn't a huge priority. I mostly want to test the recent node.js versions.

Amusingly due to the clock() granularity (due to Spectre mitigations), xxhsum -bi0 has only a few possible entries.

Ok I think I figured everything out. I want to add various versions of the Emscripten SDK to the matrix but that isn't a huge priority. I mostly want to test the recent node.js versions. Amusingly due to the `clock()` granularity (due to Spectre mitigations), `xxhsum -bi0` has only a few possible entries.
easyaspi314 commented 2023-03-14 17:17:29 -05:00 (Migrated from github.com)

This seems to be ready to merge, porting the NEON code was dead simple. Sorry I had to do it the old fashioned way because CI tests were ignored.

This seems to be ready to merge, porting the NEON code was dead simple. Sorry I had to do it the old fashioned way because CI tests were ignored.
Cyan4973 (Migrated from github.com) reviewed 2023-03-14 17:45:31 -05:00
xxhash.h
Cyan4973 (Migrated from github.com) commented 2023-03-14 17:45:31 -05:00

minor nit: alignment

minor nit: alignment
Cyan4973 (Migrated from github.com) reviewed 2023-03-14 17:46:20 -05:00
xxhash.h
Cyan4973 (Migrated from github.com) commented 2023-03-14 17:46:20 -05:00

doc: since what ?
was more explanation expected there ?

doc: since what ? was more explanation expected there ?
Cyan4973 (Migrated from github.com) approved these changes 2023-03-14 17:49:21 -05:00
Cyan4973 (Migrated from github.com) left a comment

This looks good to me.
It fits well in existing code base.

I would recommend publishing a set of benchmark as part of this PR description, as it can serve as reference for future users of the WASM128 code path.

This looks good to me. It fits well in existing code base. I would recommend publishing a set of benchmark as part of this PR description, as it can serve as reference for future users of the `WASM128` code path.
easyaspi314 (Migrated from github.com) reviewed 2023-03-14 17:53:09 -05:00
xxhash.h
easyaspi314 (Migrated from github.com) commented 2023-03-14 17:53:08 -05:00

That was probably my cat lol

That was probably my cat lol
easyaspi314 (Migrated from github.com) reviewed 2023-03-14 17:53:21 -05:00
xxhash.h
easyaspi314 (Migrated from github.com) commented 2023-03-14 17:53:20 -05:00

Fixed

Fixed
easyaspi314 commented 2023-03-14 18:21:33 -05:00 (Migrated from github.com)

This looks good to me. It fits well in existing code base.

I would recommend publishing a set of benchmark as part of this PR description, as it can serve as reference for future users of the WASM128 code path.

A basic xxhsum -b benchmark is up now.

Also, I managed to get a bench on Firefox. It isn't as impressive but it is still a solid improvement on x86 and just a slight decrease on aarch64.

> This looks good to me. It fits well in existing code base. > > I would recommend publishing a set of benchmark as part of this PR description, as it can serve as reference for future users of the `WASM128` code path. A basic xxhsum -b benchmark is up now. Also, I managed to get a bench on Firefox. It isn't as impressive but it is still a solid improvement on x86 and just a slight decrease on aarch64.
👍 1
easyaspi314 commented 2023-03-14 20:38:55 -05:00 (Migrated from github.com)

Actually....I have an idea.

emcc emulates NEON intrinsics with SIMDe, and the intrinsics are so similar that I am considering scrapping the intrinsics and using NEON with some slight modifications.

I literally get the same BETTER performance if I just replace the multiply in scrambleAcc and put an asm guard on the secret pointer to keep it from turning it into literals.

Screw these intrinsics. I'm putting wasm on XXH_NEON with a couple of ifdefs.

Actually....I have an idea. emcc emulates NEON intrinsics with `SIMDe`, and the intrinsics are so similar that I am considering scrapping the intrinsics and using NEON with some slight modifications. I literally get ~~the same~~ BETTER performance if I just replace the multiply in scrambleAcc and put an asm guard on the secret pointer to keep it from turning it into literals. Screw these intrinsics. I'm putting wasm on `XXH_NEON` with a couple of `ifdefs`.
easyaspi314 commented 2023-03-14 23:05:39 -05:00 (Migrated from github.com)

🦀 SIMD128 intrinsics are GONE 🦀

SIMDe generates the same thing, and I was actually able to make it faster, now reaching 16.3 GB/s on my phone. (Mostly due to stopping Clang from constant folding kSecret).

I also found a stupidly obvious optimization to XXH3_scrambleAcc_neon. 🤦‍♀️

I will get some new benchmarks tomorrow and probably squash.

🦀 SIMD128 intrinsics are GONE 🦀 SIMDe generates the same thing, and I was actually able to make it *faster*, now reaching 16.3 GB/s on my phone. (Mostly due to stopping Clang from constant folding kSecret). I also found a stupidly obvious optimization to `XXH3_scrambleAcc_neon`. 🤦‍♀️ I will get some new benchmarks tomorrow and probably squash.
👍 1
easyaspi314 commented 2023-03-15 04:17:32 -05:00 (Migrated from github.com)

Apparently I can't figure out how git rebase works and my commits doubled lol

Edit: fixed. Aside from the CI test commit there is no trace of the intrinsics now 😏

Apparently I can't figure out how `git rebase` works and my commits doubled lol Edit: fixed. Aside from the CI test commit there is no trace of the intrinsics now 😏
easyaspi314 commented 2023-03-15 04:58:58 -05:00 (Migrated from github.com)

Ok so apparently aarch64 Firefox has slowed down even more.

However, I am not that concerned since aarch64 is likely going to be using V8 or WebKit anyways — Firefox for Android has like a 0.6% usage share lol, and it is still a reasonable speed. Looking over the source code it might be that the compiler can't hoist the shuffle masks out of the loop, but there isn't much I can do. Shuffle masks are inlined with the instruction. The only ones that would probably use it are ARM Macs and AArch64 Windows/Linux users.

Also I want to see how Safari (WebKit) on iOS fares since emrun can host a web server. Apparently WebKit doesn't support SIMD.

Ok so apparently aarch64 Firefox has slowed down even more. However, I am not that concerned since aarch64 is likely going to be using V8 or WebKit anyways — Firefox for Android has like a 0.6% usage share lol, and it is still a reasonable speed. Looking over the source code it might be that the compiler can't hoist the shuffle masks out of the loop, but there isn't much I can do. Shuffle masks are inlined with the instruction. The only ones that would probably use it are ARM Macs and AArch64 Windows/Linux users. ~~Also I want to see how Safari (WebKit) on iOS fares since `emrun` can host a web server.~~ Apparently WebKit doesn't support SIMD.
easyaspi314 commented 2023-03-16 00:57:46 -05:00 (Migrated from github.com)

The last commit is just documentation.

The last commit is just documentation.
Cyan4973 commented 2023-06-30 03:03:56 -05:00 (Migrated from github.com)

Coming back to this topic, trying to re-acquire the context.

Initially, this PR was providing a new vector code path for WASM.
Doing so would improve performance on WASM target, which currently can only use the scalar code path.
This code was initially reviewed, and accepted.

Quickly after, @easyaspi314 discovered that the WASM compiler emcc could actually ingest NEON intrinsics directly, resulting in similar performance, but removing the need for additional dedicated code path, which is good for maintenance.
The PR was then substantially altered to take advantage of this property, resulting in a generally shortened implementation.

Then, it was discovered that the "similar performance" (as WASM SIMD128) statement is not entirely correct.
This is true when running with node.js for aarch64 target.
But when it comes to x64, some performance is lost (compared to WASM SIMD128), because NEON intrinsics are different enough from AVX ones. The counter to this point is that using NEON intrinsics for WASM still delivers some hefty speed benefit compared to scalar, so the PR is still a net progress.
Another problematic issue is that Firefox for Android actually suffers from the new NEON intrinsic code path, even as the local target is aarch64, to the point of being even slower than scalar code path, making this PR no longer a "guaranteed positive". The counter to this point is to state that Firefox has probably a negligible presence on aarch64.
It would have been appropriate therefore to complete the picture by presenting benchmarks using another system (Chrome maybe?) that is presumed more representative of aarch64 scenario.

Anyway, this is what I recall from memory, and since this PR's content has changed substantially since it was first validated, it deserves another review.

@easyaspi314 , is that a correct assessment of the situation ?

Coming back to this topic, trying to re-acquire the context. Initially, this PR was providing a new vector code path for WASM. Doing so would improve performance on WASM target, which currently can only use the `scalar` code path. This code was initially reviewed, and accepted. Quickly after, @easyaspi314 discovered that the WASM compiler `emcc` could actually ingest NEON intrinsics directly, resulting in similar performance, but removing the need for additional dedicated code path, which is good for maintenance. The PR was then substantially altered to take advantage of this property, resulting in a generally shortened implementation. Then, it was discovered that the "similar performance" (as WASM SIMD128) statement is not entirely correct. This is true when running with node.js for `aarch64` target. But when it comes to `x64`, some performance is lost (compared to WASM SIMD128), because NEON intrinsics are different enough from AVX ones. The counter to this point is that using NEON intrinsics for WASM still delivers some hefty speed benefit compared to `scalar`, so the PR is still a net progress. Another problematic issue is that Firefox for Android actually suffers from the new NEON intrinsic code path, even as the local target is `aarch64`, to the point of being _even slower_ than `scalar` code path, making this PR no longer a "guaranteed positive". The counter to this point is to state that Firefox has probably a negligible presence on `aarch64`. It would have been appropriate therefore to complete the picture by presenting benchmarks using another system (Chrome maybe?) that is presumed more representative of `aarch64` scenario. Anyway, this is what I recall from memory, and since this PR's content has changed substantially since it was first validated, it deserves another review. @easyaspi314 , is that a correct assessment of the situation ?
easyaspi314 commented 2023-06-30 14:50:54 -05:00 (Migrated from github.com)

V8 and NodeJS use Chrome's JavaScript engine.

Browser Render Engine JS engine
Chrome, Chromium Chromium V8
Edge Chromium V8
Opera, Brave, etc Chromium V8
Node.JS N/A V8
Firefox Gecko SpiderMonkey
Safari WebKit JavaScriptCore

I am not that concerned about the Firefox AArch64 problem:

  1. Firefox AArch64 is quite uncommon:
  • Apple mandates that all iOS browsers use WebKit, so Firefox isn't using its own engine
  • Firefox on Android is niche, with only 100M downloads despite being on the Play Store since 2010
  1. The only thing preventing the same 16 GB/s as V8 is a missed optimization we don't have control over (see also - MSVC x86). However, browsers are constantly updated so the performance will only improve even if the binaries aren't recompiled. (Judging from the performance it looks like SIMD isn't/wasn't ready for AArch64 and it was scalarized)
  2. Since it is so uncommon, I would put it in the category of the XXH32 AArch64 issue, where we give up max performance on specific targets (this case Firefox instead of the A53 and X1) for better/more balanced performance on the majority

I should update and run some new benchmarks on the latest versions, as well as get a Safari benchmark as iOS 16 should have SIMD now. (Unfortunately my MBP is out of commission so Safari x64 might be tricky to bench unless I can find a WebKit impl for Windows)

As for validation, the NEON intrinsics we use are literally a 1:1 translation to SIMD128 aside from the umlal needing an additional add, so it is as optimal as wasm intrinsics unless I am missing something.

V8 and NodeJS use Chrome's JavaScript engine. | Browser | Render Engine | JS engine | | :--- | :-- | :-- | | Chrome, Chromium | Chromium | V8 | | Edge | Chromium | V8 | | Opera, Brave, etc | Chromium | V8 | | Node.JS | N/A | V8 | | Firefox | Gecko | SpiderMonkey | | Safari | WebKit | JavaScriptCore | **I am not that concerned about the Firefox AArch64 problem:** 1. Firefox AArch64 is quite uncommon: - Apple mandates that all iOS browsers use WebKit, so Firefox isn't using its own engine - Firefox on Android is niche, with only 100M downloads despite being on the Play Store since 2010 2. The only thing preventing the same 16 GB/s as V8 is a missed optimization we don't have control over (see also - MSVC x86). However, browsers are constantly updated so the performance will only improve even if the binaries aren't recompiled. (Judging from the performance it looks like SIMD isn't/wasn't ready for AArch64 and it was scalarized) 3. Since it is so uncommon, I would put it in the category of the XXH32 AArch64 issue, where we give up max performance on specific targets (this case Firefox instead of the A53 and X1) for better/more balanced performance on the majority I should update and run some new benchmarks on the latest versions, as well as get a Safari benchmark as iOS 16 should have SIMD now. (Unfortunately my MBP is out of commission so Safari x64 might be tricky to bench unless I can find a WebKit impl for Windows) As for validation, the NEON intrinsics we use are literally a 1:1 translation to SIMD128 aside from the umlal needing an additional add, so it is as optimal as wasm intrinsics unless I am missing something.
👍 1
easyaspi314 commented 2023-06-30 18:08:37 -05:00 (Migrated from github.com)

Found that I can run WebKit via Epiphany on Linux.

WebKit x64 isn't doing too hot.

AMD Ryzen 5 3600
Ubuntu 22.04 LTS on WSL2 
Epiphany Technology Preview 45.alpha-2-g08992b26f+
Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.4 Safari/605.1.15

scalar

this.program 0.8.1 by Yann Collet
compiled as 32-bit wasm/asmjs little endian with Clang 17.0.0
Sample of 100 KB...
 1#XXH32                         :     102400 ->    62087 it/s ( 6063.2 MB/s)
 3#XXH64                         :     102400 ->   117833 it/s (11507.1 MB/s)
 5#XXH3_64b                      :     102400 ->    79804 it/s ( 7793.4 MB/s)
11#XXH128                        :     102400 ->    77467 it/s ( 7565.1 MB/s)

simd128

this.program 0.8.1 by Yann Collet
compiled as 32-bit wasm/asmjs + simd128 little endian with Clang 17.0.0
Sample of 100 KB...
 1#XXH32                         :     102400 ->    62770 it/s ( 6129.9 MB/s)
 3#XXH64                         :     102400 ->   123634 it/s (12073.7 MB/s)
 5#XXH3_64b                      :     102400 ->    50899 it/s ( 4970.7 MB/s)
11#XXH128                        :     102400 ->    51623 it/s ( 5041.3 MB/s)

Apparently, this is because WebKit converts extmul to zero extend + full multiply without a special case, and then it is forced to scalarize because it will only emit a i64x2 multiply if AVX-512 is enabled. (NEON, x64). I might open up a PR/issue for this to be added because this is pathetic 💀

However, I don't quite understand what is up with Firefox's aarch64 jit. I might have to boot up my raspi so I can get the JIT debugger. (Edit: Really? No Linux aarch64 builds?! Do I seriously have to install tiny11 for this?)

Found that I can run WebKit via Epiphany on Linux. WebKit x64 isn't doing too hot. ``` AMD Ryzen 5 3600 Ubuntu 22.04 LTS on WSL2 Epiphany Technology Preview 45.alpha-2-g08992b26f+ Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.4 Safari/605.1.15 ``` scalar ```c this.program 0.8.1 by Yann Collet compiled as 32-bit wasm/asmjs little endian with Clang 17.0.0 Sample of 100 KB... 1#XXH32 : 102400 -> 62087 it/s ( 6063.2 MB/s) 3#XXH64 : 102400 -> 117833 it/s (11507.1 MB/s) 5#XXH3_64b : 102400 -> 79804 it/s ( 7793.4 MB/s) 11#XXH128 : 102400 -> 77467 it/s ( 7565.1 MB/s) ``` simd128 ```c this.program 0.8.1 by Yann Collet compiled as 32-bit wasm/asmjs + simd128 little endian with Clang 17.0.0 Sample of 100 KB... 1#XXH32 : 102400 -> 62770 it/s ( 6129.9 MB/s) 3#XXH64 : 102400 -> 123634 it/s (12073.7 MB/s) 5#XXH3_64b : 102400 -> 50899 it/s ( 4970.7 MB/s) 11#XXH128 : 102400 -> 51623 it/s ( 5041.3 MB/s) ``` Apparently, this is because WebKit [converts `extmul` to zero extend + full multiply](https://github.com/WebKit/WebKit/blob/cecd69451d07f0ab7eb2fbba3661ef19fa085072/Source/JavaScriptCore/wasm/WasmAirIRGenerator64.cpp#L1712) without a special case, and then it is forced to scalarize because it will only emit a i64x2 multiply if AVX-512 is enabled. ([NEON](https://github.com/WebKit/WebKit/blob/cecd69451d07f0ab7eb2fbba3661ef19fa085072/Source/JavaScriptCore/assembler/ARM64Assembler.h#L1667), [x64](https://github.com/WebKit/WebKit/blob/cecd69451d07f0ab7eb2fbba3661ef19fa085072/Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h#L2897)). I might open up a PR/issue for this to be added because this is pathetic 💀 However, I don't quite understand what is up with Firefox's aarch64 jit. I might have to boot up my raspi so I can get the JIT debugger. *(Edit: Really? No Linux aarch64 builds?! Do I seriously have to install tiny11 for this?)*
👍 1
easyaspi314 commented 2023-07-01 01:15:09 -05:00 (Migrated from github.com)

Well I got a build of Firefox's jsshell for AArch64 that can dump native code, but the standard emcc wrappers don't work with it. I might have to write my own WASM glue. 💀

I am truly baffled on how the performance is worse, as the reason isn't blatant like WebKit — all instructions should be lowered to the direct NEON counterparts.

Also I tried adjusting the JIT parameters on Firefox, to no avail. And it isn't baseline jit being used because when I disable optimizing jit it is only 2.9 GB/s.

Well I got a build of Firefox's jsshell for AArch64 that can dump native code, but the standard emcc wrappers don't work with it. I might have to write my own WASM glue. 💀 I am truly baffled on how the performance is worse, as the reason isn't blatant like WebKit — all instructions should be lowered to the direct NEON counterparts. Also I tried adjusting the JIT parameters on Firefox, to no avail. And it isn't baseline jit being used because when I disable optimizing jit it is only 2.9 GB/s.
Cyan4973 commented 2023-07-05 03:16:04 -05:00 (Migrated from github.com)

So, to summarize the situation,
this PR influences performance of XXH3 when compiled for WASM,
under Node.JS, it's much faster (2x-3x),
but under Web Browser environment, whether it is Chrome of Firefox,
it's only a bit faster for x64 cpus (+50%)
but it's slower for aarch64 cpus (-20-30%)
even though the WASM code path uses NEON intrinsics.

Correct ?

So, to summarize the situation, this PR influences performance of `XXH3` when compiled for WASM, under Node.JS, it's much faster (2x-3x), but under Web Browser environment, whether it is Chrome of Firefox, it's only a bit faster for x64 cpus (+50%) but it's slower for `aarch64` cpus (-20-30%) even though the WASM code path uses `NEON` intrinsics. Correct ?
easyaspi314 commented 2023-07-05 23:23:50 -05:00 (Migrated from github.com)

So, to summarize the situation,
this PR influences performance of XXH3 when compiled for WASM,
under Node.JS, it's much faster (2x-3x),
but under Web Browser environment, whether it is Chrome of Firefox,
it's only a bit faster for x64 cpus (+50%)
but it's slower for aarch64 cpus (-20-30%)
even though the WASM code path uses NEON intrinsics.

Correct ?

Basically although a little mixed up.

For all intents and purposes, Chrome and Node.JS are the same. I have confirmed that the benchmarks are nearly identical because they use the same JS engine (V8) under the hood. Chrome pairs V8 with the Chromium HTML renderer, Node.JS pairs V8 with an OS interface.

Rough diagram of the browser situation:

browser engines (1).png

"Common name" JS Engine Paired HTML engine Uses aarch64 x64 Notes
V8 V8 Chromium Node.js, Chrome, Chromium (Browser), Opera, Brave, Vivaldi, etc much faster faster Most popular, most important
Firefox SpiderMonkey Gecko Firefox, Tor Browser, Pale Moon, etc slower faster AArch64 rare, simple JIT bug?
WebKit JavaScriptCore WebKit Safari, Epiphany slower (speculation) slower SIMD is new, simple JIT bug.

Firefox and Chrome (and derivatives) are on a rapid release cycle, and the only time you see an old JS engine is an older version of Node.JS (which doesn't matter because it is already a performance improvement), an unsupported OS, or someone who refuses to update. You rarely come into the issue where someone is stuck with an old toolchain as well because of how emcc is distributed.

WebKit only recently added SIMD128 so it is unsurprising that there are a few kinks. This one is very simple and could be fixed in a few hours once I figure out how to contribute.

Firefox AArch64 is probably also a simple bug that I can't tell from the source code itself, given how it should be a direct translation. I mostly need to find a minimal reproducible example for the codegen bug to report it.

However, the important thing is that the code that is generated is optimal on the WASM level, and this is a problem with the upstream browsers. I say merge and then we try to fix the Firefox and WebKit bugs.

Additionally, the most important target is V8 because most browsers are Chromium based, and Node.JS will affect server-side performance which is very significant on a large server.

> So, to summarize the situation, > this PR influences performance of `XXH3` when compiled for WASM, > under Node.JS, it's much faster (2x-3x), > but under Web Browser environment, whether it is Chrome of Firefox, > it's only a bit faster for x64 cpus (+50%) > but it's slower for `aarch64` cpus (-20-30%) > even though the WASM code path uses `NEON` intrinsics. > > Correct ? Basically although a little mixed up. For all intents and purposes, Chrome and Node.JS are the same. I have confirmed that the benchmarks are nearly identical because they use the same JS engine (V8) under the hood. Chrome pairs V8 with the Chromium HTML renderer, Node.JS pairs V8 with an OS interface. Rough diagram of the browser situation: ![browser engines (1).png](https://github.com/Cyan4973/xxHash/assets/6258309/19c4ab62-b44e-44ae-9f41-5dc6e1add0c5) | "Common name" | JS Engine | Paired HTML engine | Uses | aarch64 | x64 | Notes | |:--|:--|:--|:--|:--|:--|:--| | V8 | V8 | Chromium | **Node.js, Chrome**, Chromium (Browser), Opera, Brave, Vivaldi, etc | much faster | faster | Most popular, most important | | Firefox | SpiderMonkey | Gecko | **Firefox**, Tor Browser, Pale Moon, etc | **slower** | faster | AArch64 rare, simple JIT bug? | | WebKit | JavaScriptCore | WebKit | **Safari**, Epiphany | **slower (speculation)** | **slower** | SIMD is new, simple JIT bug. | Firefox and Chrome (and derivatives) are on a rapid release cycle, and the only time you see an old JS engine is an older version of Node.JS (which doesn't matter because it is already a performance improvement), an unsupported OS, or someone who refuses to update. You rarely come into the issue where someone is stuck with an old toolchain as well because of how `emcc` is distributed. WebKit only recently added SIMD128 so it is unsurprising that there are a few kinks. This one is **very simple** and could be fixed in a few hours once I figure out how to contribute. Firefox AArch64 is probably also a simple bug that I can't tell from the source code itself, given how it should be a direct translation. I mostly need to find a minimal reproducible example for the codegen bug to report it. However, the important thing is that the code that is generated is optimal on the **WASM** level, and this is a problem with the upstream browsers. I say merge and then we try to fix the Firefox and WebKit bugs. Additionally, the most important target is V8 because most browsers are Chromium based, and Node.JS will affect server-side performance which is _very significant_ on a large server.
👍 1
Cyan4973 (Migrated from github.com) reviewed 2023-07-06 04:50:59 -05:00
Makefile
@ -55,6 +55,13 @@ else
EXT =
endif
ifeq ($(NODE_JS),1)
Cyan4973 (Migrated from github.com) commented 2023-07-06 04:50:59 -05:00

This environment variable NODE_JS=1 must be set at compilation time.
It should probably be documented in README.md.

This environment variable `NODE_JS=1` must be set at compilation time. It should probably be documented in `README.md`.
Cyan4973 (Migrated from github.com) approved these changes 2023-07-06 04:52:10 -05:00
Cyan4973 commented 2023-07-09 17:06:23 -05:00 (Migrated from github.com)

I'm going to insist a bit regarding the documentation of the new NODE_JS build variable,
since I don't know if I would be able to complete it myself.
I see NODE_JS used in tests, but I don't know if this variable is meant to be used only for tests, or meant to be used only with node.js specifically, if it can ignored at (presumably) the cost of performance, etc.

I'm going to insist a bit regarding the documentation of the new `NODE_JS` build variable, since I don't know if I would be able to complete it myself. I see `NODE_JS` used in tests, but I don't know if this variable is meant to be used only for tests, or meant to be used only with node.js specifically, if it can ignored at (presumably) the cost of performance, etc.
easyaspi314 commented 2023-07-09 19:15:01 -05:00 (Migrated from github.com)

Done. I also did things a bit more correctly, added a helpful note to the make check output if it fails, and removed the RUN_ENV override.

NODE_JS adds proper detection of terminals (as the default emsdk's isatty is a no-op) and more importantly gives unrestricted access to the filesystem.

Done. I also did things a bit more correctly, added a helpful note to the make check output if it fails, and removed the `RUN_ENV` override. `NODE_JS` adds proper detection of terminals (as the default emsdk's isatty is a no-op) and more importantly gives unrestricted access to the filesystem.
👍 1
Sign in to join this conversation.
Reviewers
No reviewers
Cyan4973
Labels
Clear labels   
announcement

   
bug

   
build

   
CI
Continuous Integration tests

  
dependencies
Pull requests that update a dependency file

  
documentation

   
duplicate

   
enhancement

   
experience feedback

   
feature request

   
github_actions
Pull requests that update GitHub Actions code

  
help wanted

   
install

   
invalid

   
Long-term

   
optimization

   
portability

   
question

   
target-specific
issue triggered in a specific environment (harware, compiler, OS)

  
test issue

   
wontfix
No labels
announcement
bug
build
CI
dependencies
documentation
duplicate
enhancement
experience feedback
feature request
github_actions
help wanted
install
invalid
Long-term
optimization
portability
question
target-specific
test issue
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Cyan4973/xxHash!825
No description provided.