Skip to content

lib: reduce URL invocations on http2 origins - #48338

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
anonrig:add-url-origin-getter
Jun 7, 2023
Merged

lib: reduce URL invocations on http2 origins#48338
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
anonrig:add-url-origin-getter

Conversation

@anonrig

@anonriganonrig commented Jun 5, 2023

Copy link
Copy Markdown
Member

URL origin getter is quite a costly function. This pull request makes sure that there are only 1 js to C++ pass done for this operation.

@nodejs-github-bot

nodejs-github-bot commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
  • @nodejs/url

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Jun 5, 2023
@anonrig
anonrigforce-pushed the add-url-origin-getter branch from e6193e1 to 42e0afaCompareJune 5, 2023 01:52
aduh95
aduh95 reviewed Jun 5, 2023
View reviewed changes
Comment threadlib/internal/url.js
aduh95
aduh95 reviewed Jun 5, 2023
View reviewed changes
Comment threadlib/internal/url.js
mcollina
mcollina approved these changes Jun 5, 2023
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 5, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 5, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

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

@anonriganonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 5, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

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

juanarbol
juanarbol approved these changes Jun 5, 2023
View reviewed changes

@juanarboljuanarbol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 5, 2023
@github-actionsgithub-actionsBot mentioned this pull request Jun 6, 2023
Open
20 tasks
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 72ba099 into nodejs:mainJun 7, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator

Landed in 72ba099

This was referenced Jun 9, 2023
Open
Open
Open
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@anonrig@RafaelGSS
lib: reduce URL invocations on http2 origins
159ab66
PR-URL: #48338
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig@Ceres6
lib: reduce URL invocations on http2 origins
7465fda
PR-URL: nodejs#48338
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig@Ceres6
lib: reduce URL invocations on http2 origins
9566316
PR-URL: nodejs#48338
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@ruyadorno

ruyadorno commented Aug 29, 2023

Copy link
Copy Markdown
Member

hi @anonrig this is not landing cleanly on v18.x-staging and will need manual backport.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lemirelemirelemire left review comments
@mcollinamcollinamcollina approved these changes
@ShogunPandaShogunPandaShogunPanda approved these changes
@aduh95aduh95aduh95 approved these changes
@juanarboljuanarboljuanarbol approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@anonrig@nodejs-github-bot@ruyadorno@mcollina@ShogunPanda@lemire@aduh95@juanarbol