Conversation
|
@dnlup Would you be interested in trying to benchmark a wasm simd build? |
mcollina
left a comment
There was a problem hiding this comment.
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.