lib: clarify get byteLength #10574

Open
wants to merge 1 commit into
from

Projects

None yet

7 participants

@JacksonTian
Contributor
JacksonTian commented Jan 2, 2017 edited

The Buffer.byteLength() accepts Buffer instance as parameter, so
the code can be simplified to Buffer.byteLength(value, encoding)
without type check.

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

net, _http_outgoing, child_process

@addaleax

LGTM

@JacksonTian JacksonTian lib: clarify get byteLength
The Buffer.byteLength() accepts Buffer instance as parameter, so
the code can be simplified to Buffer.byteLength(value, encoding)
without type check.
ab3a532
@mscdex
Contributor
mscdex commented Jan 2, 2017

Did you check relevant benchmarks with this? Since byteLength() is quite large and thus currently uninlineable, I could see this possibly causing a perf hit for non-strings?

@cjihrig
cjihrig approved these changes Jan 2, 2017 View changes

LGTM if performance remains steady.

@jasnell
jasnell approved these changes Jan 3, 2017 View changes
@mscdex
Contributor
mscdex commented Jan 4, 2017 edited

Ok, I just checked with benchmark/http/simple.js (e.g. type=buffer length=4 chunks=4 c=50) for example and with that there does seem to be a slight perf hit with the changes in this PR (I'm seeing roughly 2-7% without doing a long drawn-out comparison).

Because of that I'd have to say -1 for me.

The changes to the net module are probably ok, because most people probably don't use bytesWritten that often. Besides, there's already a forEach() in that getter so it's already somewhat slower because of that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment