src: use simdutf for converting externalized builtins to UTF-16 #46119

addaleax · 2023-01-06T17:54:16Z

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.

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.

nodejs-github-bot · 2023-01-06T17:54:20Z

Review requested:

@nodejs/gyp
@nodejs/startup

test/cctest/test_util.cc

anonrig

perfect. lgtm.

…o UTF-16

nodejs-github-bot · 2023-01-06T23:37:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48887/

addaleax · 2023-01-09T15:49:05Z

@nodejs/cpp-reviewers Anybody else want to take a look here?

joyeecheung · 2023-01-09T15:57:45Z

src/node_builtins.cc

+      utf8source.data(),
+      utf8source.length(),
+      reinterpret_cast<char16_t*>(out->data()));
+  out->resize(u16_length);


Is this bit really necessary?

What kind of alternative would you suggest? Or is this about the resize() call?

joyeecheung · 2023-01-09T16:08:22Z

src/node_builtins.cc

+  Add(id, source);
+}
+
+bool BuiltinLoader::Add(const char* id, std::string_view utf8source) {


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?))

So, UnionBytes that supports storing UTF-8 directly?

Fwiw, I’ll merge this in order to unblock #45942, but I’m still happy to iterate further here.

A union for everything ;)

nodejs-github-bot · 2023-01-10T11:25:20Z

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>

nodejs/node#46119

* 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>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 6, 2023

addaleax mentioned this pull request Jan 6, 2023

src: make BuiltinLoader threadsafe and non-global #45942

Merged

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2023

anonrig reviewed Jan 6, 2023

View reviewed changes

test/cctest/test_util.cc Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

anonrig approved these changes Jan 6, 2023

View reviewed changes

fixup! src: use simdutf for converting externalized builtins to UTF-16

0693b9f

addaleax added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 6, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2023

This comment was marked as outdated.

Sign in to view

fixup! fixup! src: use simdutf for converting externalized builtins t…

123ba08

…o UTF-16

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2023

anonrig approved these changes Jan 7, 2023

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Jan 7, 2023

addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 9, 2023

joyeecheung reviewed Jan 9, 2023

View reviewed changes

jasnell approved these changes Jan 9, 2023

View reviewed changes

RafaelGSS approved these changes Jan 9, 2023

View reviewed changes

RaisinTen approved these changes Jan 10, 2023

View reviewed changes

addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 10, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 10, 2023

nodejs-github-bot merged commit 21fb98e into nodejs:main Jan 10, 2023

addaleax deleted the simdutf-for-builtin-conversions branch January 10, 2023 11:26

targos mentioned this pull request Jan 10, 2023

build: add GitHub Action for coverage with --without-intl #37954

Closed

This was referenced Jan 11, 2023

CI Reliability 2023-01-11 nodejs/reliability#480

Open

CI Reliability 2023-01-12 nodejs/reliability#481

Open

CI Reliability 2023-01-13 nodejs/reliability#482

Open

CI Reliability 2023-01-14 nodejs/reliability#483

Open

anonrig mentioned this pull request Jan 18, 2023

segfault with shared builtin #46235

Closed

RafaelGSS mentioned this pull request Jan 20, 2023

v19.5.0 proposal #46286

Merged

juanarbol mentioned this pull request Jan 28, 2023

V18.14.0 proposal #46396

Merged

codebytere added a commit to electron/electron that referenced this pull request Feb 6, 2023

src: use simdutf for converting externalized builtins to UTF-16

b0fa353

nodejs/node#46119

codebytere added a commit to electron/electron that referenced this pull request Feb 6, 2023

src: use simdutf for converting externalized builtins to UTF-16

a8fa0d4

nodejs/node#46119

codebytere added a commit to electron/electron that referenced this pull request Feb 6, 2023

src: use simdutf for converting externalized builtins to UTF-16

fbc5120

nodejs/node#46119

joyeecheung mentioned this pull request Mar 13, 2023

src: register external references for source code #47055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use simdutf for converting externalized builtins to UTF-16 #46119

src: use simdutf for converting externalized builtins to UTF-16 #46119

addaleax commented Jan 6, 2023

nodejs-github-bot commented Jan 6, 2023

This comment was marked as outdated.

anonrig left a comment •

edited

This comment was marked as outdated.

nodejs-github-bot commented Jan 6, 2023

addaleax commented Jan 9, 2023

joyeecheung Jan 9, 2023

addaleax Jan 9, 2023

joyeecheung Jan 9, 2023

addaleax Jan 9, 2023

addaleax Jan 10, 2023

joyeecheung Jan 12, 2023

nodejs-github-bot commented Jan 10, 2023

src: use simdutf for converting externalized builtins to UTF-16 #46119

src: use simdutf for converting externalized builtins to UTF-16 #46119

Conversation

addaleax commented Jan 6, 2023

nodejs-github-bot commented Jan 6, 2023

This comment was marked as outdated.

anonrig left a comment • edited

Choose a reason for hiding this comment

This comment was marked as outdated.

nodejs-github-bot commented Jan 6, 2023

addaleax commented Jan 9, 2023

joyeecheung Jan 9, 2023

Choose a reason for hiding this comment

addaleax Jan 9, 2023

Choose a reason for hiding this comment

joyeecheung Jan 9, 2023

Choose a reason for hiding this comment

addaleax Jan 9, 2023

Choose a reason for hiding this comment

addaleax Jan 10, 2023

Choose a reason for hiding this comment

joyeecheung Jan 12, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 10, 2023

anonrig left a comment •

edited