Skip to content

http: cleanup ClientRequest oncreate - #36862

Closed
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:client-oncreate
Closed

http: cleanup ClientRequest oncreate#36862
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:client-oncreate

Conversation

@ronag

@ronagronag commented Jan 10, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Iterative work towards cleaning up ClientRequest.

@ronagronag added the http Issues or PRs related to the http subsystem. label Jan 10, 2021
@ronag
ronag requested review from BridgeAR and TrottJanuary 10, 2021 10:04
@ronag

ronag commented Jan 10, 2021

Copy link
Copy Markdown
MemberAuthor

@nodejs/http

mscdex
mscdex reviewed Jan 11, 2021
View reviewed changes
Comment threadlib/_http_client.js
@nodejs-github-bot

nodejs-github-bot commented Jan 11, 2021

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jan 11, 2021

Copy link
Copy Markdown
Collaborator

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

Trott
Trott approved these changes Jan 15, 2021
View reviewed changes

@TrottTrott left a comment •
edited
Loading

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.

Looks good to me. Should we run a subset of the http benchmarks before landing? @mscdex

@nodejs/http

mcollina
mcollina approved these changes Jan 15, 2021
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

dnlup
dnlup approved these changes Jan 15, 2021
View reviewed changes

@dnlupdnlup left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@dnlup

dnlup commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

@Trott is benchmark-node-micro-benchmarks the CI job for running benchmarks in this list?

@ronag

ronag commented Jan 15, 2021

Copy link
Copy Markdown
MemberAuthor

@dnlup Do you want to try and land this?

@dnlup

dnlup commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

@dnlup Do you want to try and land this?

@ronag Yes 👍🏻 . I'll run the benchmark job on the http subsystem first.

@dnlup

dnlup commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/897/

@dnlup

dnlup commented Jan 16, 2021

Copy link
Copy Markdown
Contributor

The job is pretty long. It's still running. I don't see performance regressions so far, inspecting the job console output.

dnlup pushed a commit that referenced this pull request Jan 16, 2021
@ronag@dnlup
http: cleanup ClientRequest oncreate
963dec3
PR-URL: #36862
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daniele Belardi <dwon.dnl@gmail.com>
@dnlup

dnlup commented Jan 16, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Landed in 963dec3

@dnlupdnlup closed this Jan 16, 2021
@Trott

Trott commented Jan 19, 2021

Copy link
Copy Markdown
Member

@dnlup The http benchmarks take a long time to run, so it's usually a good idea to use the filter option to run a subset of the http benchmarks. In this case, I think using request as the value for FILTER would have limited the benchmarks to just client-request-body.js and create-clientrequest.js.

I'll open a draft revert PR so we can see that benchmark. Assuming no significant performance issues, I'll close the draft revert PR.

@dnlup

dnlup commented Jan 19, 2021

Copy link
Copy Markdown
Contributor

@dnlup The http benchmarks take a long time to run, so it's usually a good idea to use the filter option to run a subset of the http benchmarks. In this case, I think using request as the value for FILTER would have limited the benchmarks to just client-request-body.js and create-clientrequest.js.

I'll open a draft revert PR so we can see that benchmark. Assuming no significant performance issues, I'll close the draft revert PR.

@Trott thank you for the explanation. I apologize for the extra work I caused.

ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
@ronag@ruyadorno
http: cleanup ClientRequest oncreate
80051ab
PR-URL: #36862
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daniele Belardi <dwon.dnl@gmail.com>
@ruyadornoruyadorno mentioned this pull request Jan 22, 2021
Merged
@Trott

Trott commented Feb 14, 2021

Copy link
Copy Markdown
Member

@Trott thank you for the explanation. I apologize for the extra work I caused.

No worries at all. I was only letting you know in case this is an area where you plan on doing other similar work and may need to run another http benchmark.

targos pushed a commit that referenced this pull request May 1, 2021
@ronag@targos
http: cleanup ClientRequest oncreate
e7bc379
PR-URL: #36862
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daniele Belardi <dwon.dnl@gmail.com>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@TrottTrottTrott approved these changes
@BridgeARBridgeARAwaiting requested review from BridgeAR
@mscdexmscdexAwaiting requested review from mscdex
@benjamingrbenjamingrAwaiting requested review from benjamingr
+1 more reviewer
@dnlupdnlupdnlup approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

httpIssues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ronag@nodejs-github-bot@dnlup@Trott@mcollina@mscdex