[v18.x backport] replace url parser with ada by anonrig · Pull Request #47435 · nodejs/node · GitHub

Backports the following pull requests:

Review requested:

@nodejs/actions
@nodejs/crypto
@nodejs/gyp
@nodejs/loaders
@nodejs/modules
@nodejs/node-api
@nodejs/tsc
@nodejs/url
@nodejs/vm

PR-URL: nodejs#46410 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

PR-URL: nodejs#46550 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: nodejs#46550 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: nodejs#46547 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: nodejs#46547 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#46547 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#46784 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#46853 Fixes: nodejs#46850 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Xuguang Mei <meixuguang@gmail.com>

Use `std::string_view` for process.report code and related code, drop a few unnecessary `std::to_string` calls, and use `MaybeStackBuffer` instead of `MallocedBuffer`, all in order to avoid unnecessary heap allocations. PR-URL: nodejs#46723 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#46736 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

PR-URL: #46410 Backport-PR-URL: #47435 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

PR-URL: #46550 Backport-PR-URL: #47435 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: #46550 Backport-PR-URL: #47435 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: #46547 Backport-PR-URL: #47435 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: #46547 Backport-PR-URL: #47435 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #46547 Backport-PR-URL: #47435 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #46784 Backport-PR-URL: #47435 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #46853 Backport-PR-URL: #47435 Fixes: #46850 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Xuguang Mei <meixuguang@gmail.com>

Use `std::string_view` for process.report code and related code, drop a few unnecessary `std::to_string` calls, and use `MaybeStackBuffer` instead of `MallocedBuffer`, all in order to avoid unnecessary heap allocations. PR-URL: #46723 Backport-PR-URL: #47435 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: #46736 Backport-PR-URL: #47435 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 62cbddd...a39dd37

The static is likely to harm you a little here...

Variables declared at block scope with the specifier static or thread_local have static or thread storage duration but are initialized the first time control passes through their declaration (unless their initialization is zero- or constant-initialization, which can be performed before the block is first entered). On all further calls, the declaration is skipped. If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once). Note: usual implementations of this feature use variants of the double-checked locking pattern, which reduces runtime overhead for already-initialized local statics to a single non-atomic boolean comparison.

constexpr static would be better if you can.

(Note that just dropping static would be worse.)

You're right. This change was done in the following PR of @addaleax #46723.

(No need to make changes now that this has landed.)

@lemire This change is still in the main branch. Would you like to open a pull request?

Let me see what I can do...

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. v18.x labels Apr 5, 2023

anonrig force-pushed the backport-ada-v18 branch from c90eadd to af1b1e5 Compare April 5, 2023 21:21

anonrig requested a review from danielleadams April 5, 2023 21:25

anonrig force-pushed the backport-ada-v18 branch from 202e5f9 to af1b1e5 Compare April 5, 2023 21:48

anonrig mentioned this pull request Apr 5, 2023

src: use string_view for report and related code #46723

Merged

danielleadams approved these changes Apr 11, 2023

View reviewed changes

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

anonrig force-pushed the backport-ada-v18 branch from 8a38aed to ccb5b4f Compare April 11, 2023 12:56

This comment was marked as outdated.

Sign in to view

anonrig and others added 11 commits April 11, 2023 10:10

tools: fix update-ada script

0c9a8a1

PR-URL: nodejs#46550 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

test: update web-platform tests for url

78b3490

PR-URL: nodejs#46547 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

url: improve performance by removing host

b2289da

PR-URL: nodejs#46547 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

deps: update ada to 1.0.4

7df525b

PR-URL: nodejs#46853 Fixes: nodejs#46850 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Xuguang Mei <meixuguang@gmail.com>

url: simplify and improve url formatting

aae4141

PR-URL: nodejs#46736 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

anonrig force-pushed the backport-ada-v18 branch from ccb5b4f to aae4141 Compare April 11, 2023 15:42

github-actions Bot mentioned this pull request Apr 12, 2023

CI Reliability 2023-04-12 nodejs/reliability#541

Open

31 tasks

danielleadams pushed a commit that referenced this pull request Apr 12, 2023

url: simplify and improve url formatting

a39dd37

PR-URL: #46736 Backport-PR-URL: #47435 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams closed this Apr 12, 2023

lemire reviewed Apr 12, 2023

View reviewed changes

github-actions Bot mentioned this pull request Apr 13, 2023

CI Reliability 2023-04-13 nodejs/reliability#542

Open

36 tasks

-              std::string EscapeJsonChars(const std::string& str) {
-                const std::string control_symbols[0x20] = {
+              std::string EscapeJsonChars(std::string_view str) {
+                static const std::string_view control_symbols[0x20] = {

[v18.x backport] replace url parser with ada by anonrig · Pull Request #47435 · nodejs/node · GitHub

Uh oh!

Uh oh!

[v18.x backport] replace url parser with ada - #47435

[v18.x backport] replace url parser with ada#47435
anonrig wants to merge 11 commits into
nodejs:v18.x-stagingfrom
anonrig:backport-ada-v18

anonrig commented Apr 5, 2023 •
edited
Loading

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 5, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Apr 11, 2023

Uh oh!

danielleadams commented Apr 12, 2023

Uh oh!

lemire Apr 12, 2023

Uh oh!

lemire Apr 12, 2023

Uh oh!

anonrig Apr 12, 2023

Uh oh!

lemire Apr 12, 2023

Uh oh!

anonrig Apr 12, 2023

Uh oh!

lemire Apr 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[v18.x backport] replace url parser with ada by anonrig · Pull Request #47435 · nodejs/node · GitHub

Uh oh!

Uh oh!

[v18.x backport] replace url parser with ada - #47435

[v18.x backport] replace url parser with ada#47435anonrig wants to merge 11 commits intonodejs:v18.x-stagingnodejs/node:v18.x-stagingfrom anonrig:backport-ada-v18anonrig/node:backport-ada-v18Copy head branch name to clipboard

Conversation

anonrig commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 5, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Apr 11, 2023

Uh oh!

danielleadams commented Apr 12, 2023

Uh oh!

lemire Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

lemire Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

lemire Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

lemire Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[v18.x backport] replace url parser with ada#47435
anonrig wants to merge 11 commits into
nodejs:v18.x-stagingfrom
anonrig:backport-ada-v18

anonrig commented Apr 5, 2023 •
edited
Loading