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

Remove duplicated code #27985

Closed
wants to merge 2 commits into from
Closed

Remove duplicated code #27985

wants to merge 2 commits into from

Conversation

jahirfiquitiva
Copy link

@jahirfiquitiva jahirfiquitiva commented May 30, 2019

Description

I was checking at the source code of the buffer.js file, and noticed that certain parts of the code could be simplified.

Basically, you were checking for some lowercase expected text and then doing "something". If the text was not lowercase, you would make it lowercase and then do the exact same check and the exact same "something".

So, I thought, why not ensuring the text is already lowercase and then do the checks only once?

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
Notes

Unfortunately I didn't read or noticed the Pull Request contribution guidelines before creating it, so I hope there's no issue into getting the changes merged (if they deserve to be merged).

Thanks a ton

@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label May 30, 2019
@ChALkeR
Copy link
Member

ChALkeR commented May 30, 2019

That function was written that way because of performance considerations.
See e.g. #12361 which touched that code. That was fairly long ago, though.

How does this change affect buffers benchmarks? See the doc on how to run those.

@ChALkeR ChALkeR added the performance Issues and PRs related to the performance of Node.js. label May 30, 2019
@refack
Copy link
Contributor

refack commented May 30, 2019

Hello @jahirfiquitiva and thank you for the contribution!

I've gone ahead and started an automated benchmark compare job, I'm interested to see what it comes up with...

Good luck 🤞

@jahirfiquitiva
Copy link
Author

jahirfiquitiva commented May 30, 2019

@ChALkeR thanks for the info, I also thought there might be a reason why it was left that way, but I didn't know about the benchmarks.

@refack thank you so much! I was trying to run one by one but your solution is much better.

Excited to know those results 🙂

@refack
Copy link
Contributor

refack commented May 31, 2019

Seems like that duplicate code still has a performance benefit.

Summary of only significant results.

  confidence improvement (*) (**) (***)
buffers/buffer-write-string.js n=10000000 len=10 args='' encoding='UCS-2' *** 8.85 % ±2.35% ±3.15% ±4.12%
buffers/buffer-write-string.js n=10000000 len=10 args='offset' encoding='UCS-2' *** 6.87 % ±3.67% ±4.94% ±6.55%
buffers/buffer-write-string.js n=10000000 len=10 args='offset+length' encoding='UCS-2' *** 6.69 % ±2.23% ±3.00% ±3.97%
buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='UCS-2' *** 5.01 % ±1.73% ±2.31% ±3.01%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset' encoding='UCS-2' *** 4.29 % ±2.23% ±2.98% ±3.89%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset+length' encoding='UCS-2' *** 4.20 % ±0.91% ±1.22% ±1.58%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset' encoding='latin1' *** -9.91 % ±2.37% ±3.16% ±4.14%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset+length' encoding='latin1' *** -9.80 % ±3.37% ±4.52% ±5.95%
buffers/buffer-write-string.js n=10000000 len=10 args='offset' encoding='binary' *** -9.63 % ±2.36% ±3.14% ±4.09%
buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='latin1' *** -9.39 % ±3.17% ±4.22% ±5.51%
buffers/buffer-write-string.js n=10000000 len=10 args='offset+length' encoding='latin1' *** -9.30 % ±1.69% ±2.27% ±3.01%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset+length' encoding='binary' *** -9.26 % ±2.78% ±3.72% ±4.87%
buffers/buffer-write-string.js n=10000000 len=10 args='offset' encoding='latin1' *** -9.12 % ±0.96% ±1.28% ±1.66%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset' encoding='binary' *** -9.10 % ±2.45% ±3.26% ±4.24%
buffers/buffer-write-string.js n=10000000 len=10 args='' encoding='ascii' *** -9.03 % ±1.64% ±2.20% ±2.90%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset+length' encoding='ascii' *** -8.92 % ±3.29% ±4.41% ±5.79%
buffers/buffer-write-string.js n=10000000 len=10 args='offset+length' encoding='ascii' *** -8.83 % ±2.19% ±2.92% ±3.81%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset' encoding='ascii' *** -8.82 % ±2.49% ±3.34% ±4.41%
buffers/buffer-write-string.js n=10000000 len=10 args='offset+length' encoding='binary' *** -8.55 % ±2.23% ±2.97% ±3.87%
buffers/buffer-write-string.js n=10000000 len=10 args='' encoding='binary' *** -8.53 % ±2.65% ±3.57% ±4.71%
buffers/buffer-write-string.js n=10000000 len=10 args='' encoding='utf8' *** -8.08 % ±1.93% ±2.57% ±3.35%
buffers/buffer-write-string.js n=10000000 len=10 args='offset+length' encoding='utf8' *** -6.90 % ±1.30% ±1.73% ±2.26%
buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='ascii' *** -6.88 % ±3.23% ±4.30% ±5.60%
buffers/buffer-write-string.js n=10000000 len=10 args='offset' encoding='utf8' *** -6.73 % ±2.08% ±2.77% ±3.60%
buffers/buffer-write-string.js n=10000000 len=10 args='' encoding='hex' *** -6.33 % ±3.43% ±4.57% ±5.94%
buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='binary' *** -5.83 % ±2.19% ±2.94% ±3.87%
buffers/buffer-write-string.js n=10000000 len=10 args='offset+length' encoding='hex' *** -5.21 % ±2.32% ±3.09% ±4.02%
buffers/buffer-write-string.js n=10000000 len=10 args='offset' encoding='hex' *** -4.39 % ±0.82% ±1.09% ±1.42%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset+length' encoding='utf16le' *** -4.36 % ±1.96% ±2.62% ±3.41%
buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='utf8' *** -3.71 % ±1.98% ±2.64% ±3.46%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset' encoding='utf8' *** -2.52 % ±1.13% ±1.51% ±1.96%
buffers/buffer-write-string.js n=10000000 len=10 args='' encoding='latin1' *** -10.90 % ±2.05% ±2.74% ±3.58%
buffers/buffer-write-string.js n=10000000 len=10 args='offset' encoding='ascii' *** -10.84 % ±3.32% ±4.43% ±5.78%
buffers/buffer-write-string.js n=10000000 len=10 args='offset+length' encoding='utf16le' *** -10.70 % ±3.01% ±4.00% ±5.21%
buffers/buffer-write-string.js n=10000000 len=10 args='' encoding='utf16le' *** -10.47 % ±1.88% ±2.53% ±3.35%
buffers/buffer-write-string.js n=10000000 len=10 args='offset' encoding='utf16le' *** -10.27 % ±2.12% ±2.84% ±3.71%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset+length' encoding='utf8' *** -1.61 % ±0.80% ±1.06% ±1.38%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset' encoding='utf16le' ** -2.68 % ±1.68% ±2.23% ±2.91%
buffers/buffer-write-string.js n=10000000 len=2048 args='offset' encoding='hex' ** -0.98 % ±0.65% ±0.88% ±1.15%
buffers/buffer-write.js n=1000000 type='UInt16BE' buffer='fast' * -6.52 % ±4.95% ±6.62% ±8.67%
buffers/buffer-write.js n=1000000 type='UInt32BE' buffer='slow' * -6.30 % ±5.91% ±7.90% ±10.35%
buffers/buffer-write.js n=1000000 type='Int16LE' buffer='fast' * -5.20 % ±4.11% ±5.49% ±7.18%
buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='utf16le' * -2.84 % ±2.76% ±3.69% ±4.85%

All results FTR - https://gist.github.com/refack/abe23c6941cf9599f46146296e331cf0

@jahirfiquitiva
Copy link
Author

Interesting ... Thanks for sharing @refack

@ChALkeR
Copy link
Member

ChALkeR commented May 31, 2019

Yes, this seems to noticeably slow down small buffer writes with all specified encodings, except for UCS-2, and to improve the speed for UCS-2 writes somehow. Probably because it's the only one that was not normalized to ucs2 in tests.

@jahirfiquitiva
Copy link
Author

Understood... I'll let you take the final decision 🙂

@Trott
Copy link
Member

Trott commented Jun 1, 2019

Hi, @jahirfiquitiva! Thanks again for the pull request. I'm going to close this one out, but I hope this doesn't discourage you from improving the code base in other ways!

@Trott Trott closed this Jun 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants