-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: use simdutf for converting externalized builtins to UTF-16 #46119
src: use simdutf for converting externalized builtins to UTF-16 #46119
Conversation
Remove the dependency on ICU for this part, as well as the hacky way of converting embedder main sources to UTF-8 via V8 APIs. Allow `UnionBytes` to own the memory its pointing to in order to simplify the code on the `BuiltinLoader` side.
Review requested:
|
This comment was marked as outdated.
This comment was marked as outdated.
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.
perfect. lgtm.
This comment was marked as outdated.
This comment was marked as outdated.
@nodejs/cpp-reviewers Anybody else want to take a look here? |
utf8source.data(), | ||
utf8source.length(), | ||
reinterpret_cast<char16_t*>(out->data())); | ||
out->resize(u16_length); |
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.
Is this bit really necessary?
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.
What kind of alternative would you suggest? Or is this about the resize()
call?
Add(id, source); | ||
} | ||
|
||
bool BuiltinLoader::Add(const char* id, std::string_view utf8source) { |
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.
I wonder if we can just create a UnionBytes
constructor that takes a std::string_view
that does this? (then I guess that UnionBytes
can own the vector?))
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.
So, UnionBytes
that supports storing UTF-8 directly?
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.
Fwiw, I’ll merge this in order to unblock #45942, but I’m still happy to iterate further here.
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.
A union for everything ;)
Landed in 21fb98e |
Remove the dependency on ICU for this part, as well as the hacky way of converting embedder main sources to UTF-8 via V8 APIs. Allow `UnionBytes` to own the memory its pointing to in order to simplify the code on the `BuiltinLoader` side. PR-URL: nodejs#46119 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
Remove the dependency on ICU for this part, as well as the hacky way of converting embedder main sources to UTF-8 via V8 APIs. Allow `UnionBytes` to own the memory its pointing to in order to simplify the code on the `BuiltinLoader` side. PR-URL: #46119 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
Remove the dependency on ICU for this part, as well as the hacky way of converting embedder main sources to UTF-8 via V8 APIs. Allow `UnionBytes` to own the memory its pointing to in order to simplify the code on the `BuiltinLoader` side. PR-URL: #46119 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
Remove the dependency on ICU for this part, as well as the hacky way of converting embedder main sources to UTF-8 via V8 APIs. Allow `UnionBytes` to own the memory its pointing to in order to simplify the code on the `BuiltinLoader` side. PR-URL: #46119 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
* chore: bump node in DEPS to v18.14.0 * src: add support for externally shared js builtins nodejs/node#44376 * test: fix test broken under --node-builtin-modules-path nodejs/node#45894 * build: add option to disable shared readonly heap nodejs/node#45887 * src: remove unnecessary semicolons nodejs/node#46171 * src: remove return after abort nodejs/node#46172 * chore: fixup patch indices * test_runner: parse yaml nodejs/node#45815 * src: use simdutf for converting externalized builtins to UTF-16 nodejs/node#46119 * src: rename internal module declaration as internal bindings nodejs/node#45551 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* chore: bump node in DEPS to v18.14.0 * src: add support for externally shared js builtins nodejs/node#44376 * test: fix test broken under --node-builtin-modules-path nodejs/node#45894 * build: add option to disable shared readonly heap nodejs/node#45887 * src: remove unnecessary semicolons nodejs/node#46171 * src: remove return after abort nodejs/node#46172 * chore: fixup patch indices * test_runner: parse yaml nodejs/node#45815 * src: use simdutf for converting externalized builtins to UTF-16 nodejs/node#46119 * src: rename internal module declaration as internal bindings nodejs/node#45551 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* chore: bump node in DEPS to v18.14.0 * src: add support for externally shared js builtins nodejs/node#44376 * test: fix test broken under --node-builtin-modules-path nodejs/node#45894 * build: add option to disable shared readonly heap nodejs/node#45887 * src: remove unnecessary semicolons nodejs/node#46171 * src: remove return after abort nodejs/node#46172 * chore: fixup patch indices * test_runner: parse yaml nodejs/node#45815 * src: use simdutf for converting externalized builtins to UTF-16 nodejs/node#46119 * src: rename internal module declaration as internal bindings nodejs/node#45551 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* chore: bump node in DEPS to v18.14.0 * src: add support for externally shared js builtins nodejs/node#44376 * test: fix test broken under --node-builtin-modules-path nodejs/node#45894 * build: add option to disable shared readonly heap nodejs/node#45887 * src: remove unnecessary semicolons nodejs/node#46171 * src: remove return after abort nodejs/node#46172 * chore: fixup patch indices * test_runner: parse yaml nodejs/node#45815 * src: use simdutf for converting externalized builtins to UTF-16 nodejs/node#46119 * src: rename internal module declaration as internal bindings nodejs/node#45551 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* chore: bump node in DEPS to v18.14.0 * src: add support for externally shared js builtins nodejs/node#44376 * test: fix test broken under --node-builtin-modules-path nodejs/node#45894 * build: add option to disable shared readonly heap nodejs/node#45887 * src: remove unnecessary semicolons nodejs/node#46171 * src: remove return after abort nodejs/node#46172 * chore: fixup patch indices * test_runner: parse yaml nodejs/node#45815 * src: use simdutf for converting externalized builtins to UTF-16 nodejs/node#46119 * src: rename internal module declaration as internal bindings nodejs/node#45551 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Remove the dependency on ICU for this part, as well as the hacky way of converting embedder main sources to UTF-8 via V8 APIs. Allow
UnionBytes
to own the memory its pointing to in order to simplify the code on theBuiltinLoader
side.