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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 11 commits into from
Closed

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

wants to merge 11 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 5, 2023 •
edited
Loading

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot 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 Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Apr 5, 2023
@anonrig anonrig mentioned this pull request Apr 5, 2023
Merged
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
anonrig and others added 11 commits April 11, 2023 10:10
@anonrig
deps: add ada as a dependency
ea03c6f 
PR-URL: nodejs#46410
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@anonrig
url: replace url-parser with ada
b227448 
PR-URL: nodejs#46410
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@anonrig
tools: fix update-ada script
0c9a8a1 
PR-URL: nodejs#46550
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@anonrig @lemire
deps: update ada to v1.0.1
47e8f4e 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#46550
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@anonrig
test: update web-platform tests for url
78b3490 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@anonrig @lemire
url: fix url spec compliance issues
205678a 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@anonrig
url: improve performance by removing host
b2289da 
PR-URL: nodejs#46547
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@nodejs-github-bot @anonrig
deps: update ada to 1.0.3
a74ca49 
PR-URL: nodejs#46784
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot @anonrig
deps: update ada to 1.0.4
7df525b 
PR-URL: nodejs#46853
Fixes: nodejs#46850
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
@addaleax @anonrig
src: use string_view for report and related code
76fde06 
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@anonrig
url: simplify and improve url formatting
aae4141 
PR-URL: nodejs#46736
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Apr 12, 2023
Open
31 tasks
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
deps: add ada as a dependency
0bc4c17 
PR-URL: #46410
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
url: replace url-parser with ada
e926354 
PR-URL: #46410
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
tools: fix update-ada script
958da45 
PR-URL: #46550
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @lemire @danielleadams
deps: update ada to v1.0.1
484c4f6 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #46550
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
test: update web-platform tests for url
972bdee 
PR-URL: #46547
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @lemire @danielleadams
url: fix url spec compliance issues
0c67a7a 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #46547
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
url: improve performance by removing host
01b6525 
PR-URL: #46547
Backport-PR-URL: #47435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@nodejs-github-bot @danielleadams
deps: update ada to 1.0.3
94f8353 
PR-URL: #46784
Backport-PR-URL: #47435
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@nodejs-github-bot @danielleadams
deps: update ada to 1.0.4
e5c4d69 
PR-URL: #46853
Backport-PR-URL: #47435
Fixes: #46850
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@addaleax @danielleadams
src: use string_view for report and related code
e4cd81b 
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 12, 2023
@anonrig @danielleadams
url: simplify and improve url formatting
a39dd37 
PR-URL: #46736
Backport-PR-URL: #47435
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams
Copy link
Member

Landed in 62cbddd...a39dd37

lemire
lemire reviewed Apr 12, 2023
View reviewed changes
src/json_utils.cc
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] = {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

constexpr static would be better if you can.

(Note that just dropping static would be worse.)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Let me see what I can do...

@github-actions github-actions bot mentioned this pull request Apr 13, 2023
Open
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemire lemire lemire left review comments

@danielleadams danielleadams danielleadams approved these changes

Assignees
No one assigned
Labels
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 Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anonrig @nodejs-github-bot @danielleadams @lemire @addaleax