-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
[v18.x backport] replace url parser with ada #47435
Conversation
Review requested:
|
c90eadd
to
af1b1e5
Compare
202e5f9
to
af1b1e5
Compare
8a38aed
to
ccb5b4f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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#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>
ccb5b4f
to
aae4141
Compare
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: #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: #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>
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>
Landed in 62cbddd...a39dd37 |
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] = { |
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.
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.
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.
constexpr static would be better if you can.
(Note that just dropping static would be worse.)
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.
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.
(No need to make changes now that this has landed.)
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.
@lemire This change is still in the main
branch. Would you like to open a pull request?
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.
Let me see what I can do...
Backports the following pull requests: