-
Notifications
You must be signed in to change notification settings - Fork 545
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
perf: enable wasm simd #735
Conversation
@dnlup Would you be interested in trying to benchmark a wasm simd build? |
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.
@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. |
Ouch! |
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. |
What node version are we using for benchmark CI? |
Good catch, https://github.com/nodejs/undici/blob/main/.github/workflows/bench.yml#L18 |
@dnlup would you mind running this with the latest benchark suite on master? |
The |
@dnlup any update on perf numbers? |
Sorry, I was stuck trying to understand what is wrong with the CI. Running locally:
|
Is SIMD still experimental on Node 16? |
@dnlup Can we ship a separate simd assembly and do a runtime check if it's available and load accordingly? |
I haven't checked the docs, but removing the flag still crashes the script, so I think so. |
We can modify the build script, yes. By available, is it ok to assume that Node has been launched with the cli option? |
Or alternatively. Always try to use the simd version first and if compilation fails then fallback to non simd? |
That sounds better |
I like this one too. Are we ok with introducing it as a runtime dep? |
I'm ok with it. But the feature detect is so trivial we might as well consider inlining it with a ref comment. |
I prefer not to add additional runtime dependencies |
I think V8 9.1 no longer requires the |
You can also update the README with updated bench info. |
@mcollina PTAL. Also could you give us a bench run on this PR? |
@dnlup is this ready for review? |
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. |
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? |
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
|
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? |
simd makes it faster by roughly ~10% across various runs. |
Regarding the benchmarks I'm also confused to why the difference between single connection and 50 connections is so large... |
me too |
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. |
Ci fails |
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. |
Co-authored-by: Robert Nagy <ronagy@icloud.com>
I too noticed the same ~10% improvement with SIMD in my runs while working on PR #796 with Node.js v16.1.0 |
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 |
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. |
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 <ronagy@icloud.com> * client: fallback to non-simd on all errors * fixup: re-enable jest * fixup Co-authored-by: Daniele Belardi <dwon.dnl@gmail.com>
wasm simd is still experimental but looking at the performance could be interesting already.