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 infinite loop in isEncoding function #3546

Closed
wants to merge 1 commit into from

Conversation

fernandezpablo85
Copy link

the code is simpler and easier to read.

the code is simpler and easier to read.
@cjihrig
Copy link
Contributor

cjihrig commented Oct 27, 2015

I think @trevnorris did this as a performance enhancement.

@Fishrock123 Fishrock123 added the buffer Issues and PRs related to the buffer subsystem. label Oct 27, 2015
@bnoordhuis
Copy link
Member

It's a (fairly significant) performance enhancement, see 59dac01. Thanks but we won't be taking this change.

@bnoordhuis bnoordhuis closed this Oct 27, 2015
@fernandezpablo85
Copy link
Author

Have you guys run some kind of benchmark to verify that statement? Here's a simple jsperf that shows otherwise:

screen shot 2015-10-27 at 3 43 28 pm

http://jsperf.com/isencoding

@Nibbler999
Copy link
Contributor

You need to test using 'utf8' as the encoding.

https://jsperf.com/isencoding/2

@trevnorris
Copy link
Contributor

I've analyzed the generated code from the optimizing compiler and the current code is more optimized. One problem your benchmark has is DCE (dead code elimination). It's easy for v8 to optimize out the simple loop that the benchmark is running in. In fact, the code is stable and has no side effects. Meaning the function call can be completely removed.

The current implementation will in fact be slower if the passed string is not an encoding, but it's better to optimize for the common case.

@fernandezpablo85
Copy link
Author

@trevnorris yeah, I added a console.log to avoid DCE but then the results were dominated by that call and they were indistinguishable. IMHO that's another argument in favor of better readability and simplicity.

@trevnorris
Copy link
Contributor

@fernandezpablo85 In general I agree with that sentiment, but core holds to a different standard. If we always chose readability over performance then we would be sorely lacking today. For examples you can look at the HTTPParser, nextTickQueue or how we bypass zero fill while allocating new Uint8Arrays for Buffers to understand the measures we've taken to ensure node runs much faster than if we were to use the "usual" approach.

That being said, isEncoding() is probably not a hot path, but relatively it's also a simple perf boost. Also, it's code is modeled after Buffer.byteLength(), Buffer#toString(), Buffer#indexOf() and Buffer#write(). Most of which are considered hot paths. So at that point we could even argue that leaving it as is maintains uniformity.

Thanks much for your contribution, and I look forward to being able to review more PRs from you in the future!

@fernandezpablo85
Copy link
Author

No problem, thanks for the explanation.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants