buffer:Performance increase for buffer-indexof

[adrian-nitu-92]

Checklist

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

Affected core subsystem(s)

buffer

Description of change

We are currently analyzing the buffer-indexof performance and noticed that a
considerable amount of time is spent in the isNan() function.
By postponing the isNan check we noticed a performance boost.

Signed-off-by: Adrian Nițu [email protected]

[cjihrig]

Do you have performance numbers for this?

[mscdex]

Missing indentation

[adrian-nitu-92]

Performance numbers available at, as measured on:

uname -a Linux anitu-desk 4.6.2-1-ARCH #1 SMP PREEMPT Wed Jun 8 08:40:59 CEST 2016 x86_64 GNU/Linux 

---

cat /proc/cpuinfo processor : 0 vendor_id : GenuineIntel cpu family : 6 model : 60 model name : Intel(R) Core(TM) i7-4790K CPU @ 4.00GHz stepping : 3 microcode : 0x1c cpu MHz : 4389.218 cache size : 8192 KB .... 

[adrian-nitu-92]

Implemented requested changes :D

[mscdex]

What if we just changed the isNaN(byteOffset) to byteOffset !== byteOffset and nothing else? That should simplify the changes while achieving the original performance goal.

[adrian-nitu-92]

I looked into that, it gets slightly lower performance(under 1% lower).
The middle ground would be something like the general case gets a free bypass and then use the original code :)

[adrian-nitu-92]

Any comments on the (typeof byteOffset === 'string') condition?

It feels really awkward :D

[mscdex]

typeof byteOffset === 'string' is just fine since it's checking for an optional argument. That particular conditional is checking if byteOffset was not provided, but the encoding was. This can be checked only because the two parameters are different types.

[adrian-nitu-92]

So I gather I should just simplify the patch according to mscdex's comments and resend my patch

[jasnell]

@adrian-nitu-92 ... sorry for the delay, updating this PR would be the way to do it. If you're still interested in moving this forward, simply add the new commits here and we'll take a look! :-)

[adrian-nitu-92]

Thank you for the feedback, I simplified my patch

[Trott]

@nodejs/buffer @nodejs/benchmarking @mscdex

[imyller]

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

[jasnell]

LGTM

[ThePrimeagen]

@adrian-nitu-92 could you put a comment explaining what you are doing? Its clever, and clever often leads to misunderstanding. A quick comment should alleviate what looks like a bug.

The comment below is somewhat misleading, you could also just clean that up.

[jasnell]

Is this still something that should happen?

[mscdex]

I think adding a comment above the conditional might be a good idea. Either way it LGTM.

[mhdawson]

I agree adding a comment explaining what and how the check accomplishes would be useful. Its not obvious otherwise

[trevnorris]

Would be worth it to run this with Number.isNaN(byteOffset). Since the operation just above it already coerces the value, should be able to do a more direct check.

[mscdex]

@trevnorris I'm not sure it'd be worth it since it needs to be converted anyway?

[trevnorris]

@mscdex What I mean is that because of:

byteOffset=+byteOffset;// Coerce to Number.

we can use the more strict Numer.isNaN() rather than the generic.

[mscdex]

@trevnorris The n !== n style of NaN checking seems to be faster than Number.isNaN(n).

[trevnorris]

@mscdex Hm. Seems the code was updated to use !== between my last two comments, but I didn't look at the change before my last comment. Change looks good.

[Trott]

#12286 seems to duplicate this and include some other things (a benchmark file, a comment explaining why it's doing things this way, and implementing the same change where Number.isNan() is used in one other place in the file. I'm going to close this in favor of that PR, but feel free to comment or re-open if you think this should stay open. Thanks for the PR!