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

buffer: move checkFloat from lib into src #3763

Closed
wants to merge 1 commit into from
Closed

buffer: move checkFloat from lib into src #3763

wants to merge 1 commit into from

Conversation

matthewloring
Copy link

The type and range checks performed by this function can be done more
efficiently in native code.

/cc @trevnorris

Environment* env = Environment::GetCurrent(args);

bool noAssert = args[3]->BooleanValue();
if (!noAssert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid double negative stuff here, if you simply name it as should_assert or something like that.

Copy link
Author

Choose a reason for hiding this comment

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

It is named this way to maintain consistency with the existing JS api.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewloring I think the convention in C++ is to use snake case for local variables and I am more interested in something like this

bool should_assert = !args[3]->BooleanValue();
...
if (should_assert) {
    ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It is convention to use snake_case in native code, and since I've been bitten in the butt by it before (in buffer code actually) I'll have to agree with @thefourtheye's observation of avoiding double negative is a better option.

Also, we'll need to do some benchmarking on BooleanValue(). Last time I used it (way way back in v0.10) the performance was bad enough that we actually prefixed the check with IsUndefined(). So may look something like this:

bool should_assert = !args[3]->IsUndefined() && !args[3]->BooleanValue();

Think that's right, but it's getting late.

Copy link
Author

Choose a reason for hiding this comment

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

I believe the condition should be args[3]->IsUndefined() || !args[3]->BooleanValue(). Alternatively, we could use !! on the JS side to ensure we only get booleans at this point the same way the other arguments are type coerced value = +value; offset = offset >>> 0;. Do you have a preference between these?

Copy link
Author

Choose a reason for hiding this comment

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

In the mean time, I've fixed the case and negation issues here.

Copy link

Choose a reason for hiding this comment

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

These v8::Value::XValue methods are scheduled for deprecation in favor of the Maybe versions.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the value accesses in this function to use the new version. It looks like the non-Maybe versions are still used elsewhere throughout the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewloring From my distant memory it was indeed faster to coerce noAssert in JS. Then you can do a simple IsTrue() check, and drop the other two.

Copy link
Author

Choose a reason for hiding this comment

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

@trevnorris I've moved the boolean coercion into JS.

@thefourtheye thefourtheye added the buffer Issues and PRs related to the buffer subsystem. label Nov 11, 2015
CHECK_LE(offset + sizeof(T), ts_obj_length);
if (!noAssert) {
CHECK_NOT_OOB(offset + sizeof(T) <= ts_obj_length);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the CHECK_LE below the conditional check. We still want to abort to prevent writing to memory bits we don't have control over.

Copy link
Contributor

Choose a reason for hiding this comment

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

NM this comment. Logic will change w/ another upcoming PR.

@trevnorris
Copy link
Contributor

@matthewloring Awesome job. Thanks much for taking care of this. Has been one of those changes in my backlog that never taken the time to get to. Since this is all about micro-optimization (we'll only be shaving off 20ns or so here) there may be some experimentation we'll need to do

@trevnorris
Copy link
Contributor

@matthewloring There's going to be a collision with #3767. It only touches the location where you removed the CHECK_LE. Mind if I proceed with that one, the we rebase this one and continue to make things fast?

EDIT: Ref'd issue was this one. Has been fixed.

@matthewloring
Copy link
Author

@trevnorris It should be an easy merge conflict to resolve. You're welcome to land #3767.

checkFloat(this, val, offset, 4);
binding.writeFloatBE(this, val, offset);
noAssert = !!noAssert;
binding.writeFloatBE(this, val, offset, noAssert);
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, minor nit: do the boolean coercion in the call itself. e.g. writeFloatBE(this, val, offset, !!noAssert);

Copy link
Author

Choose a reason for hiding this comment

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

Inlined.

@trevnorris
Copy link
Contributor

Looks great. As soon as I can land the previously mentioned patch, get this rebased and CI is happy we'll be good to land.

LGTM.

T val = args[1]->NumberValue();
uint32_t offset = args[2]->Uint32Value();
CHECK_LE(offset + sizeof(T), ts_obj_length);
T val = args[1]->NumberValue(env->context()).FromMaybe(0);
Copy link

Choose a reason for hiding this comment

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

Why does this function take the T parameter? T is used as the return type for v8::Value::NumberValue::FromMaybe, which returns a double.

Copy link
Contributor

Choose a reason for hiding this comment

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

val may either be a double or float.

@trevnorris trevnorris self-assigned this Nov 12, 2015
@jasnell
Copy link
Member

jasnell commented Nov 12, 2015

LGTM

@jasnell
Copy link
Member

jasnell commented Nov 12, 2015

@trevnorris ... as an optimization, I'm inclined not to flag this for LTS but would like your input on that.

@trevnorris
Copy link
Contributor

@jasnell Sounds reasonable. I think this is low risk, but also not critical.

@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

@matthewloring
Copy link
Author

Looks like this may be integer overflow on 32-bit architectures? I can verify and put together a fix.

@trevnorris
Copy link
Contributor

@matthewloring ping me when you have a fix, or have a question, and will run CI again.

@matthewloring
Copy link
Author

@trevnorris I've added the lower bounds check which I believe to fix the problem but do not have a centos 32 vm to reproduce the initial failure on. Are there existing overflow prevention checks elsewhere inside buffer that I should emulate or does the added check CHECK_NOT_OOB(offset + sizeof(T) >= sizeof(T)); at line 745 seem good? If so, it's ready for another CI.

@trevnorris
Copy link
Contributor

@matthewloring
Copy link
Author

The test-crypto-dh.js timeouts are happening on a few machines but it should be unrelated.

@trevnorris
Copy link
Contributor

@matthewloring Just landed the other conflicting PR. Mind rebasing on latest master?

The other PR alerted me to the specific wording in the current documentation:

Set noAssert to true to skip validation of value and offset. This means
that value may be too large for the specific function and offset may be
beyond the end of the buffer leading to the values being silently dropped.

So I believe if noAssert is set and the offset is beyond the bound of the buffer we simply return early. Though I'd still leave in that specific CHECK just to make sure any future modifications don't accidentally allow writing beyond memory bounds.

@matthewloring
Copy link
Author

@trevnorris I've rebased and reinserted the CHECK_LE just as a failsafe. Should be good to go.

@trevnorris
Copy link
Contributor

@matthewloring
Copy link
Author

Failures (mostly crypto timeouts) should be unrelated flakes.

@trevnorris
Copy link
Contributor

@matthewloring We'll need to do some investigation. The call is slower than it is now. This is the fun part. :) I'll update if I find any details. Here's a simple script I'm using:

'use strict';
const ITER = 1e7;
var b = new Buffer(8);
var t = process.hrtime();
for (var i = 0; i < ITER; i++) {
  b.writeDoubleLE(0, 0, true);
}
t = process.hrtime(t);
console.log(((t[0] * 1e9 + t[1]) / ITER).toFixed(1) + ' ns/op');

(note: this is fine b/c the call into native isn't affected by the optimizing compiler like JS would be)

@matthewloring
Copy link
Author

Hmm, I can look into this as well.

@matthewloring
Copy link
Author

It looks like at least part of the slow down is caused by the new maybe versions of NumberValue and Uint32Value. I see ~53 ns/op with the maybe versions compared to ~46 ns/op with the old versions of NumberValue and Uint32Value and ~44 ns/op with the checks remaining in JS (at master).

@trevnorris
Copy link
Contributor

@matthewloring Here's a diff gaining back the lost performance: https://gist.github.com/trevnorris/3bce2822893a98e9d971

Sorry, this is where writing performant code makes things look worse.

@trevnorris
Copy link
Contributor

The new Maybe versions definitely aren't helping. Also getting the values out (e.g. NumberValue() are taking longer than I'd expect).

The type and range checks performed by this function can be done more
efficiently in native code.
@matthewloring
Copy link
Author

That change brings script down to ~42 for me. I've applied your change.

@trevnorris
Copy link
Contributor

@matthewloring
Copy link
Author

Only failure is a child process fork connection reset, seems good to me.

@trevnorris
Copy link
Contributor

Thanks much! Landed in 22478d3.

trevnorris pushed a commit that referenced this pull request Nov 20, 2015
The type and range checks performed by this function can be done more
efficiently in native code.

PR-URL: #3763
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@trevnorris trevnorris closed this Nov 20, 2015
rvagg pushed a commit that referenced this pull request Dec 5, 2015
The type and range checks performed by this function can be done more
efficiently in native code.

PR-URL: #3763
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Dec 17, 2015
@rvagg
Copy link
Member

rvagg commented Jan 15, 2016

Marking with dont-land-on-v4.x but noting that above discussion suggests that this could be a candidate if we changed our risk profile for LTS.

@matthewloring matthewloring deleted the buffer-check branch February 25, 2016 19:53
@ChALkeR ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Mar 8, 2016
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.

7 participants