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.

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

http: replace superfluous property with getter/setter #29015

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 6, 2019

Slightly reduces memory overhead by replacing superfluous property with getter/setter.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 6, 2019
@ronag ronag changed the title http: replace duplicate property with getter/setter http: replace superfluous property with getter/setter Aug 6, 2019
@ronag ronag force-pushed the http-connection branch from 3300437 to 73d0979 Compare August 6, 2019 11:58
@mscdex
Copy link
Contributor

mscdex commented Aug 6, 2019

AFAIK getters/setters like this incur noticeable overhead, so perhaps we should be adjusting the documentation to more strongly recommend socket over connection.

@mscdex
Copy link
Contributor

mscdex commented Aug 6, 2019

Also if we're making this change we should also replace all usage of .connection in core with .socket to avoid the overhead.

@ronag ronag force-pushed the http-connection branch 2 times, most recently from 551d5ec to 9fe52d4 Compare August 6, 2019 12:51
@ronag
Copy link
Member Author

ronag commented Aug 6, 2019

@mscdex better?

@ronag ronag force-pushed the http-connection branch 7 times, most recently from c16f5f6 to 80a07da Compare August 6, 2019 13:56
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the http-connection branch from 80a07da to f9f6147 Compare August 6, 2019 14:57
@jasnell jasnell added deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 7, 2019
@ronag ronag force-pushed the http-connection branch from f9f6147 to ddc441f Compare August 9, 2019 16:03
@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@Trott: this seems ready

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 17, 2019

Landed in 0daec61

@Trott Trott closed this Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants