src: use simdutf for converting externalized builtins to UTF-16 by addaleax · Pull Request #46119 · nodejs/node · GitHub

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.

Review requested:

@nodejs/gyp
@nodejs/startup

perfect. lgtm.

…o UTF-16

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

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

Is this bit really necessary?

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

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

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

Comment thread test/cctest/test_util.cc

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 outstanding review comments, 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

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

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

src: use simdutf for converting externalized builtins to UTF-16 by addaleax · Pull Request #46119 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src: use simdutf for converting externalized builtins to UTF-16#46119nodejs-github-bot merged 3 commits intonodejs:mainnodejs/node:mainfrom addaleax:simdutf-for-builtin-conversionsaddaleax/node:simdutf-for-builtin-conversionsCopy head branch name to clipboard

Conversation

addaleax commented Jan 6, 2023

Uh oh!

nodejs-github-bot commented Jan 6, 2023

Uh oh!

Uh oh!

This comment was marked as outdated.

anonrig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jan 6, 2023

Uh oh!

addaleax commented Jan 9, 2023

Uh oh!

joyeecheung Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

addaleax Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

addaleax Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

addaleax Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: use simdutf for converting externalized builtins to UTF-16#46119
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
addaleax:simdutf-for-builtin-conversions

anonrig left a comment •
edited
Loading