FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: avoid calling Buffer.byteLength multiple times by jasnell · Pull Request #7236 · nodejs/node · GitHub

/ node Public

tls: avoid calling Buffer.byteLength multiple times - #7236

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:perf-tls-convertprotocols
ConversationCommits (1)ChecksFiles changed
Closed

tls: avoid calling Buffer.byteLength multiple times#7236
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:perf-tls-convertprotocols

Conversation

jasnell commented Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

tls

Description of change

There's no reason to be calling Buffer.byteLength() twice. That's just silly.
Small perf improvement

Run 1:
tls/convertprotocols.js n=1 v6.2.1 = 11852, new = 12204 ...... -2.89%
tls/convertprotocols.js n=50000 v6.2.1 = 515660, new = 570610 ..... -9.63%

Run 2:
tls/convertprotocols.js n=1 v6.2.1 = 11729, new = 12045 ...... -2.62%
tls/convertprotocols.js n=50000 v6.2.1 = 512080, new = 637730 ..... -19.70%

@nodejs/crypto

jasnell added the tls Issues and PRs related to the tls subsystem. label Jun 8, 2016

indutny commented Jun 8, 2016

Copy link
Copy Markdown
Member

Is it optimization for the client?

jasnell commented Jun 8, 2016

Copy link
Copy Markdown
Member Author

client and server, only with ALPN or NPN are passed in as an Array. Very minor and low priority but just happened to spot it.

indutny reviewed Jun 8, 2016
View reviewed changes
Comment thread lib/tls.js Outdated

indutny Jun 8, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should this be n Array of size protocols.length?

jasnell Jun 8, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

it could be, yes.

indutny Jun 8, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

so new Array(protocols.length) should boost the micro-benchmark? 😉

jasnell Jun 8, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the difference is not even noticeable really. :-)

mscdex added the performance Issues and PRs related to the performance of Node.js. label Jun 8, 2016

jasnell commented Jun 8, 2016

Copy link
Copy Markdown
Member Author

updated!

indutny commented Jun 9, 2016

Copy link
Copy Markdown
Member

LGTM

tls: avoid calling Buffer.byteLength multiple times
e9aaabb 
There's no reason to be calling Buffer.byteLength() twice.
Small perf improvement

Run 1:
tls/convertprotocols.js n=1     v6.2.1 = 11852,  new = 12204 ......  -2.89%
tls/convertprotocols.js n=50000 v6.2.1 = 515660, new = 570610 .....  -9.63%

Run 2:
tls/convertprotocols.js n=1     v6.2.1 = 11729,  new = 12045 ......  -2.62%
tls/convertprotocols.js n=50000 v6.2.1 = 512080, new = 637730 ..... -19.70%
jasnell force-pushed the perf-tls-convertprotocols branch from c441764 to e9aaabb Compare June 21, 2016 17:06

jasnell commented Jun 21, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented Jun 21, 2016

Copy link
Copy Markdown
Member Author

Failures are unrelated. Also, it looks like we're having some CI troubles with a few bots hanging (@nodejs/build)

jasnell added a commit that referenced this pull request Jun 21, 2016
tls: avoid calling Buffer.byteLength multiple times
f3d5efa 
There's no reason to be calling Buffer.byteLength() twice.
Small perf improvement

Run 1:
tls/convertprotocols.js n=1     v6.2.1 = 11852,  new = 12204 ......  -2.89%
tls/convertprotocols.js n=50000 v6.2.1 = 515660, new = 570610 .....  -9.63%

Run 2:
tls/convertprotocols.js n=1     v6.2.1 = 11729,  new = 12045 ......  -2.62%
tls/convertprotocols.js n=50000 v6.2.1 = 512080, new = 637730 ..... -19.70%

PR-URL: #7236
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

jasnell commented Jun 21, 2016

Copy link
Copy Markdown
Member Author

Landed in f3d5efa

jasnell closed this Jun 21, 2016
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
tls: avoid calling Buffer.byteLength multiple times
f395172 
There's no reason to be calling Buffer.byteLength() twice.
Small perf improvement

Run 1:
tls/convertprotocols.js n=1     v6.2.1 = 11852,  new = 12204 ......  -2.89%
tls/convertprotocols.js n=50000 v6.2.1 = 515660, new = 570610 .....  -9.63%

Run 2:
tls/convertprotocols.js n=1     v6.2.1 = 11729,  new = 12045 ......  -2.62%
tls/convertprotocols.js n=50000 v6.2.1 = 512080, new = 637730 ..... -19.70%

PR-URL: #7236
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Fishrock123 mentioned this pull request Jun 27, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
tls: avoid calling Buffer.byteLength multiple times
77331a7 
There's no reason to be calling Buffer.byteLength() twice.
Small perf improvement

Run 1:
tls/convertprotocols.js n=1     v6.2.1 = 11852,  new = 12204 ......  -2.89%
tls/convertprotocols.js n=50000 v6.2.1 = 515660, new = 570610 .....  -9.63%

Run 2:
tls/convertprotocols.js n=1     v6.2.1 = 11729,  new = 12045 ......  -2.62%
tls/convertprotocols.js n=50000 v6.2.1 = 512080, new = 637730 ..... -19.70%

PR-URL: #7236
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Fishrock123 mentioned this pull request Jul 5, 2016
Merged
MylesBorins mentioned this pull request Jul 11, 2016
Closed

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

@jasnell lts?

MylesBorins commented Oct 10, 2016

Copy link
Copy Markdown
Contributor

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport @jasnell

jasnell commented Oct 10, 2016

Copy link
Copy Markdown
Member Author

This should be fine not to backport.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

performance Issues and PRs related to the performance of Node.js. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL