Skip to content

http: use arrow fns for lexical this in Agent #16475

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
Closed
wants to merge 1 commit into from
Closed

http: use arrow fns for lexical this in Agent #16475

wants to merge 1 commit into from

Conversation

@bengl
Copy link
Member

@bengl bengl commented Oct 25, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

http

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 25, 2017
@bengl
Copy link
Member Author

bengl commented Oct 25, 2017

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

mscdex
mscdex reviewed Oct 25, 2017
View reviewed changes
lib/_http_agent.js
this.maxSockets = this.options.maxSockets || Agent.defaultMaxSockets;
this.maxFreeSockets = this.options.maxFreeSockets || 256;

this.on('free', (socket, options) => {
Copy link
Contributor

@mscdex mscdex Oct 25, 2017

Choose a reason for hiding this comment

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

FWIW this function can be pulled out, named, and referenced here instead IIRC.

Copy link
Member Author

@bengl bengl Oct 25, 2017

Choose a reason for hiding this comment

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

Is there an advantage to pulling it out here?

benjamingr
benjamingr approved these changes Oct 25, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

LGTM, looks like there is no public API impact either.

Did you run any benchmarks on this btw?

@bengl
Copy link
Member Author

bengl commented Oct 25, 2017

@benjamingr Here's http/create-clientrequest.js (which seems to be the most relevant) on my machine.

On master:

http/create-clientrequest.js n=1000000 len=1: 1,335,671.932826184
http/create-clientrequest.js n=1000000 len=8: 1,251,649.5755732013
http/create-clientrequest.js n=1000000 len=16: 1,200,070.7172872
http/create-clientrequest.js n=1000000 len=32: 1,031,239.9860796627
http/create-clientrequest.js n=1000000 len=64: 1,104,284.672344222
http/create-clientrequest.js n=1000000 len=128: 908,874.688628734

With this patch:

http/create-clientrequest.js n=1000000 len=1: 1,269,757.9608199117
http/create-clientrequest.js n=1000000 len=8: 1,257,920.3856401495
http/create-clientrequest.js n=1000000 len=16: 1,218,403.6231561322
http/create-clientrequest.js n=1000000 len=32: 1,110,837.246530615
http/create-clientrequest.js n=1000000 len=64: 1,212,380.389657481
http/create-clientrequest.js n=1000000 len=128: 1,096,361.134963879

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Oct 25, 2017

@bengl - There are other instances of this transportations to unbound functions elsewhere in the codebase - for example in fs.js, net.js, repl.js, zlib.js etc. and few in tests. Just wondering is this the first-of-its kind and set a reference for some future code-and-learn exercise.

@bengl
Copy link
Member Author

bengl commented Oct 25, 2017

@gireeshpunathil I hadn't thought of it that way, but yes, this sort of change could be good for code-and-learn.

@bengl
Copy link
Member Author

bengl commented Nov 5, 2017

Landed in 1f045f4

@bengl bengl closed this Nov 5, 2017
bengl added a commit that referenced this pull request Nov 5, 2017
@bengl
http: use arrow fns for lexical this in Agent
1f045f4 
PR-URL: #16475
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@bengl @cjihrig
http: use arrow fns for lexical this in Agent
9c39d79 
PR-URL: nodejs#16475
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@bengl @gibfahn
http: use arrow fns for lexical this in Agent
9b8e2a6 
PR-URL: #16475
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+2 more reviewers

@mscdex mscdex mscdex left review comments

@hiroppy hiroppy hiroppy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.