Skip to content
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

deps: update Base64 SIMD library #45091

Closed
wants to merge 3 commits into from
Closed

Conversation

lucshi
Copy link
Contributor

@lucshi lucshi commented Oct 20, 2022

Update the Base64 SIMD library which include new features of AVX512 support and AVX inline assembly and several bug fixes.

Fixes: #45089

Update the Base64 SIMD library which include new features of AVX512
support and AVX inline assembly and several bug fixes.

Fixes: nodejs#45089
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Oct 20, 2022
@lucshi lucshi marked this pull request as ready for review October 20, 2022 13:46
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2022
@nodejs-github-bot

This comment was marked as outdated.

@lucshi
Copy link
Contributor Author

lucshi commented Oct 21, 2022

Were the checking errors caused by CI system issue?

@mscdex
Copy link
Contributor

mscdex commented Oct 21, 2022

Some of the failures are legitimate, for example on armv7l:

21:31:04 ../deps/base64/base64/lib/arch/ssse3/codec.c:12:10: fatal error: tmmintrin.h: No such file or directory
21:31:05    12 | #include <tmmintrin.h>
21:31:05       |          ^~~~~~~~~~~~~
21:31:05 compilation terminated.
21:31:05 make[2]: *** [deps/base64/base64.target.mk:100: /home/iojs/build/workspace/node-test-commit-arm/out/Release/obj.target/base64/deps/base64/base64/lib/arch/ssse3/codec.o] Error 1
21:31:05 make[2]: *** Waiting for unfinished jobs....
21:31:05 ../deps/base64/base64/lib/arch/sse42/codec.c:12:10: fatal error: nmmintrin.h: No such file or directory
21:31:05    12 | #include <nmmintrin.h>
21:31:05       |          ^~~~~~~~~~~~~
21:31:05 compilation terminated.
21:31:05 ../deps/base64/base64/lib/arch/sse41/codec.c:12:10: fatal error: smmintrin.h: No such file or directory
21:31:05    12 | #include <smmintrin.h>
21:31:05       |          ^~~~~~~~~~~~~
21:31:05 compilation terminated.
21:31:05 ../deps/base64/base64/lib/arch/avx/codec.c:12:10: fatal error: immintrin.h: No such file or directory
21:31:05    12 | #include <immintrin.h>
21:31:05       |          ^~~~~~~~~~~~~
21:31:05 compilation terminated.
21:31:05 make[2]: *** [deps/base64/base64.target.mk:100: /home/iojs/build/workspace/node-test-commit-arm/out/Release/obj.target/base64/deps/base64/base64/lib/arch/sse42/codec.o] Error 1
21:31:05 make[2]: *** [deps/base64/base64.target.mk:100: /home/iojs/build/workspace/node-test-commit-arm/out/Release/obj.target/base64/deps/base64/base64/lib/arch/sse41/codec.o] Error 1
21:31:05 make[2]: *** [deps/base64/base64.target.mk:100: /home/iojs/build/workspace/node-test-commit-arm/out/Release/obj.target/base64/deps/base64/base64/lib/arch/avx/codec.o] Error 1
21:31:05 ../deps/base64/base64/lib/arch/avx2/codec.c:12:10: fatal error: immintrin.h: No such file or directory
21:31:05    12 | #include <immintrin.h>
21:31:05       |          ^~~~~~~~~~~~~
21:31:05 compilation terminated.
21:31:05 make[2]: *** [deps/base64/base64.target.mk:100: /home/iojs/build/workspace/node-test-commit-arm/out/Release/obj.target/base64/deps/base64/base64/lib/arch/avx2/codec.o] Error 1

remove all feature definitons in this header and use gyp to define them
@nodejs-github-bot
Copy link
Collaborator

@lucshi
Copy link
Contributor Author

lucshi commented Oct 21, 2022

I suppose no actual errors in the CI..

@lucshi
Copy link
Contributor Author

lucshi commented Oct 24, 2022

Is there someone reviewing this PR?

@juanarbol
Copy link
Member

Ci hit a flaky test, I will run the CI again

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM

@lucshi
Copy link
Contributor Author

lucshi commented Oct 28, 2022

May I know if there was problems in CI?

@lucshi
Copy link
Contributor Author

lucshi commented Nov 1, 2022

May I know if there is some revisement needed before it could be landed. @mscdex

@lucshi
Copy link
Contributor Author

lucshi commented Nov 22, 2022

I'm wondering what blocked this PR merge. Could someone restart the CI and see if CI unstable issue is gone? @juanarbol

@bnoordhuis
Copy link
Member

@lucshi I can restart CI but you probably want to rebase on top off the main branch first.

@mhdawson
Copy link
Member

mhdawson commented Nov 23, 2022

@lucshi did you copy over the lastest in the main branch or what is tagged with v0.5.0. I'm thinking we should likely update to a known tag, but I can see that the current instructions don't say that.

@mhdawson
Copy link
Member

I can now see from the history that new automation updated recently to v0.5.0.

@mhdawson
Copy link
Member

I can see in the discussion of the automatic update this comment:

There is a Base64 update patch in PR #45091 which is approved but not merged yet. To make the latest Base64 works best, it needs changes in gyp files as in that PR.

I'm thinking we should be waiting for the PR to land in the upstream unless there is an urgent need to float the patch which might changed/not merged?

@mhdawson
Copy link
Member

@jasnell, @anonrig is there more context that I've missed which pushes for this getting landed before it does in the upstream?

@anonrig
Copy link
Member

anonrig commented Nov 23, 2022

@jasnell, @anonrig is there more context that I've missed which pushes for this getting landed before it does in the upstream?

Let's wait for base64 to do a new release, and automatically update it when the time comes (through the update script).

@lucshi
Copy link
Contributor Author

lucshi commented Nov 29, 2022

I can see in the discussion of the automatic update this comment:

There is a Base64 update patch in PR #45091 which is approved but not merged yet. To make the latest Base64 works best, it needs changes in gyp files as in that PR.

I'm thinking we should be waiting for the PR to land in the upstream unless there is an urgent need to float the patch which might changed/not merged?

Thanks! There is no much urgent needs. Let's wait for Base64 new release.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 19, 2023

The necessary changes seem to be landed in upstream.

But the maintainer @aklomp did not released it.

@lemire
@anonrig

It seems that avx512 is just implemented for encoding and fallback to AVX2 for decoding. Erm.. i have no experience with simd, but maybe this is an easy task for you @lemire? Or maybe takes an effort... Idk. I cant estimate the work. But maybe this is something that interests you?

@aklomp
Copy link

aklomp commented Nov 8, 2023

@Uzlopak I just released base64 v0.5.1 which has all the latest changes.

@lpinca lpinca closed this in f45bb80 Nov 11, 2023
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request Nov 14, 2023
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 20, 2024
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable AVX512 for Base64 encoding
10 participants