Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 685
perf: enable wasm simd#735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ronag commented Apr 12, 2021
@dnlup Would you be interested in trying to benchmark a wasm simd build? |
mcollina left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - hopefully this does not cause problems.
ronag commented Apr 13, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@mcollina I don't think we can merge this since it still requires a command line argument to enable experimental simd. Maybe Node 16? I'm just very interested in whether or not there are any performance gains. |
mcollina commented Apr 13, 2021
Ouch! |
dnlup commented Apr 13, 2021
I ran some benchmarks locally, but I couldn't see any noticeable differences atm. Not sure why benchmarks are failing, I did activate the command line option. |
dnlup commented Apr 13, 2021
ronag commented Apr 13, 2021
What node version are we using for benchmark CI? |
dnlup commented Apr 13, 2021
Good catch, https://github.com/nodejs/undici/blob/main/.github/workflows/bench.yml#L18 |
ronag commented Apr 21, 2021
@dnlup would you mind running this with the latest benchark suite on master? |
dnlup commented Apr 21, 2021
The |
ronag commented Apr 24, 2021
@dnlup any update on perf numbers? |
dnlup commented Apr 24, 2021
Sorry, I was stuck trying to understand what is wrong with the CI. Running locally:
|
ronag commented Apr 24, 2021
Is SIMD still experimental on Node 16? |
ronag commented Apr 24, 2021
@dnlup Can we ship a separate simd assembly and do a runtime check if it's available and load accordingly? |
dnlup commented Apr 24, 2021
I haven't checked the docs, but removing the flag still crashes the script, so I think so. |
dnlup commented Apr 24, 2021
We can modify the build script, yes. By available, is it ok to assume that Node has been launched with the cli option? |
ronag commented Apr 24, 2021
ronag commented Apr 24, 2021
Or alternatively. Always try to use the simd version first and if compilation fails then fallback to non simd? |
dnlup commented Apr 24, 2021
That sounds better |
dnlup commented Apr 24, 2021
I like this one too. Are we ok with introducing it as a runtime dep? |
ronag commented Apr 24, 2021
I'm ok with it. But the feature detect is so trivial we might as well consider inlining it with a ref comment. |
mcollina commented Apr 24, 2021
I prefer not to add additional runtime dependencies |
ronag commented Apr 24, 2021
I think V8 9.1 no longer requires the |
ronag commented Apr 24, 2021
ronag commented Apr 24, 2021
You can also update the README with updated bench info. |
ronag commented Apr 25, 2021
@mcollina PTAL. Also could you give us a bench run on this PR? |
ronag commented Apr 25, 2021
@dnlup is this ready for review? |
dnlup commented Apr 26, 2021
I have a few doubts:
Other than that, we are almost ready for review. As a note, benchmarks on the CI don't show any differences, unlike the ones I have run locally. |
ronag commented Apr 26, 2021
I think we can remove the simd versions of test and bench. I believe simd will be enable by default for Node 16.1 (or soonish). That way we will get it for free in CI.
No, I think we only report the simd bench with a note. This is the future.
I don't think it's a problem.
Maybe if @mcollina runs some benchmarks we can get a more accurate result? |
mcollina commented Apr 26, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I get some really fluctuating results even on my dedicated server - maybe we need to change something to how we benchmark. These are the latest numbers I got. master simd |
ronag commented Apr 26, 2021
Yes. Unsure what though... It's also weird how 50 connections makes it 150x faster? But I think we can conclude that simd makes it faster? |
mcollina commented Apr 26, 2021
simd makes it faster by roughly ~10% across various runs. |
ronag commented Apr 26, 2021
Regarding the benchmarks I'm also confused to why the difference between single connection and 50 connections is so large... |
mcollina commented Apr 26, 2021
me too |
Uh oh!
There was an error while loading. Please reload this page.
dnlup commented Apr 27, 2021
I found this looking for the error that pops up in Node 12. Not sure if it's fixable, maybe we should fallback to the non-simd version in that case too. |
ronag commented Apr 27, 2021
Ci fails |
dnlup commented Apr 27, 2021
Yes, it's because of that error I linked. I think I am going to fallback to the non-wasm build in that case too. |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Robert Nagy <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
trivikr commented May 10, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I too noticed the same ~10% improvement with SIMD in my runs while working on PR #796 with Node.js v16.1.0 |
trivikr commented May 10, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Verified that WebAssembly SIMD support is available by default from Chrome 91 as per Enabling experimental SIMD support in Chrome. Should have a new GitHub issue to track removal of The flag |
dnlup commented May 10, 2021
If I am not mistaken we decided to keep simd as the default for the same reasons you have pointed out. The separate script was my fault, I must have forgotten to remove it. Sorry for the late feedback. I am fine with either approach, though; there are good reasons for each one. |
ronag commented May 10, 2021
Always simd. |
* perf: enable wasm simd * bench: enable simd * build: add wasm simd * ci: use node 16 in benchmarks * test: add simd test script * ci: add simd bench * enable simd by default in tests and benchmarks * fix machine specs in README.md Co-authored-by: Robert Nagy <[email protected]> * client: fallback to non-simd on all errors * fixup: re-enable jest * fixup Co-authored-by: Daniele Belardi <[email protected]>
wasm simd is still experimental but looking at the performance could be interesting already.