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

fix(NODE-3630): remove float parser and test edge cases for Double #502

Merged
merged 12 commits into from Jul 6, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 41 additions & 1 deletion etc/benchmarks/main.mjs
Expand Up @@ -9,7 +9,7 @@ console.log();

////////////////////////////////////////////////////////////////////////////////////////////////////
await runner({
skip: false,
skip: true,
name: 'deserialize({ oid, string }, { validation: { utf8: false } })',
iterations,
setup(libs) {
Expand Down Expand Up @@ -58,6 +58,46 @@ await runner({
}
});

await runner({
skip: true,
name: 'Double Serialization',
iterations,
run(i, bson) {
bson.lib.serialize({ d: 2.3 });
}
});

await runner({
skip: false,
name: 'Double Deserialization',
iterations,
setup(libs) {
const bson = getCurrentLocalBSON(libs);
return bson.lib.serialize({ d: 2.3 });
},
run(i, bson, serialized_double) {
bson.lib.deserialize(serialized_double);
}
});

await runner({
skip: false,
name: 'Many Doubles Deserialization',
iterations,
setup(libs) {
const bson = getCurrentLocalBSON(libs);
let doubles = Object.fromEntries(
Array.from({ length: 1000 }, i => {
return [`a_${i}`, 2.3];
})
);
return bson.lib.serialize(doubles);
},
run(i, bson, serialized_doubles) {
bson.lib.deserialize(serialized_doubles);
}
});

// End
console.log(
'Total time taken to benchmark:',
Expand Down
152 changes: 0 additions & 152 deletions src/float_parser.ts

This file was deleted.

5 changes: 3 additions & 2 deletions src/parser/deserializer.ts
Expand Up @@ -197,6 +197,7 @@ function deserializeObject(
let isPossibleDBRef = isArray ? false : null;

// While we have more left data left keep parsing
const dataview = new DataView(buffer.buffer, buffer.byteOffset, buffer.byteLength);
while (!done) {
// Read the type
const elementType = buffer[index++];
Expand Down Expand Up @@ -263,10 +264,10 @@ function deserializeObject(
(buffer[index++] << 16) |
(buffer[index++] << 24);
} else if (elementType === constants.BSON_DATA_NUMBER && promoteValues === false) {
value = new Double(buffer.readDoubleLE(index));
value = new Double(dataview.getFloat64(index, true));
index = index + 8;
} else if (elementType === constants.BSON_DATA_NUMBER) {
value = buffer.readDoubleLE(index);
value = dataview.getFloat64(index, true);
index = index + 8;
} else if (elementType === constants.BSON_DATA_DATE) {
const lowBits =
Expand Down
13 changes: 10 additions & 3 deletions src/parser/serializer.ts
Expand Up @@ -9,7 +9,6 @@ import type { Double } from '../double';
import { ensureBuffer } from '../ensure_buffer';
import { BSONError, BSONTypeError } from '../error';
import { isBSONType } from '../extended_json';
import { writeIEEE754 } from '../float_parser';
import type { Int32 } from '../int_32';
import { Long } from '../long';
import { Map } from '../map';
Expand Down Expand Up @@ -79,6 +78,12 @@ function serializeString(
return index;
}

const SPACE_FOR_FLOAT64 = new Uint8Array(8);
const DV_FOR_FLOAT64 = new DataView(
SPACE_FOR_FLOAT64.buffer,
SPACE_FOR_FLOAT64.byteOffset,
SPACE_FOR_FLOAT64.byteLength
);
function serializeNumber(
buffer: Buffer,
key: string,
Expand Down Expand Up @@ -119,7 +124,8 @@ function serializeNumber(
index = index + numberOfWrittenBytes;
buffer[index++] = 0;
// Write float
writeIEEE754(buffer, value, index, 'little', 52, 8);
DV_FOR_FLOAT64.setFloat64(0, value, true);
buffer.set(SPACE_FOR_FLOAT64, index);
// Adjust index
index = index + 8;
}
Expand Down Expand Up @@ -487,7 +493,8 @@ function serializeDouble(
buffer[index++] = 0;

// Write float
writeIEEE754(buffer, value.value, index, 'little', 52, 8);
DV_FOR_FLOAT64.setFloat64(0, value.value, true);
buffer.set(SPACE_FOR_FLOAT64, index);

// Adjust index
index = index + 8;
Expand Down
5 changes: 0 additions & 5 deletions test/node/bson_corpus.spec.test.js
Expand Up @@ -121,11 +121,6 @@ describe('BSON Corpus', function () {
describe('valid-bson', function () {
for (const v of valid) {
it(v.description, function () {
if (v.description === 'NaN with payload') {
// TODO(NODE-3630): remove custom float parser so we can handle the NaN payload data
this.skip();
}

if (
v.description === 'All BSON types' &&
scenario._filename === 'multi-type-deprecated'
Expand Down
99 changes: 77 additions & 22 deletions test/node/double_tests.js
Expand Up @@ -3,36 +3,91 @@
const BSON = require('../register-bson');
const Double = BSON.Double;

describe('Double', function () {
describe('Constructor', function () {
var value = 42.3456;
describe('BSON Double Precision', function () {
context('class Double', function () {
describe('constructor()', function () {
const value = 42.3456;

it('Primitive number', function (done) {
expect(new Double(value).valueOf()).to.equal(value);
done();
it('Primitive number', function (done) {
expect(new Double(value).valueOf()).to.equal(value);
done();
});

it('Number object', function (done) {
expect(new Double(new Number(value)).valueOf()).to.equal(value);
done();
dariakp marked this conversation as resolved.
Show resolved Hide resolved
});
});

it('Number object', function (done) {
expect(new Double(new Number(value)).valueOf()).to.equal(value);
done();
describe('#toString()', () => {
it('should serialize to a string', () => {
const testNumber = Math.random() * Number.MAX_VALUE;
const double = new Double(testNumber);
expect(double.toString()).to.equal(testNumber.toString());
});

const testRadices = [2, 8, 10, 16, 22];

for (const radix of testRadices) {
it(`should support radix argument: ${radix}`, () => {
const testNumber = Math.random() * Number.MAX_VALUE;
const double = new Double(testNumber);
expect(double.toString(radix)).to.equal(testNumber.toString(radix));
});
}
});
});

describe('toString', () => {
it('should serialize to a string', () => {
const testNumber = Math.random() * Number.MAX_VALUE;
const double = new Double(testNumber);
expect(double.toString()).to.equal(testNumber.toString());
function serializeThenDeserialize(value) {
const serializedDouble = BSON.serialize({ d: value });
const deserializedDouble = BSON.deserialize(serializedDouble, { promoteValues: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the significance of promoteValues: false here? what happens with promoteValues: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so we get the raw BSON instead of the value as a double; this lets us check for payload in the BSON buffer

Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment above this line explaining that? also @nbbeeken is there a more precise option we can use than the general promoteValues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately Doubles do not have a dedicated promote option, promoteValues: false is the only way to get the deserializer to return instanceof Double. chaos

return deserializedDouble.d;
}

const testCases = [
{ name: 'Infinity', doubleVal: new Double(Infinity), testVal: Infinity },
{ name: '-Infinity', doubleVal: new Double(-Infinity), testVal: -Infinity },
{ name: 'Number.EPSILON', doubleVal: new Double(Number.EPSILON), testVal: Number.EPSILON },
{ name: 'Zero', doubleVal: new Double(0), testVal: 0 },
{ name: 'Negative Zero', doubleVal: new Double(-0), testVal: -0 },
{ name: 'NaN', doubleVal: new Double(NaN), testVal: NaN }
];

for (const { name, doubleVal, testVal } of testCases) {
it(`should preserve the input value ${name} in Double serialize-deserialize roundtrip`, () => {
const roundTrippedVal = serializeThenDeserialize(doubleVal);
expect(Object.is(doubleVal.value, testVal)).to.be.true;
expect(Object.is(roundTrippedVal.value, doubleVal.value)).to.be.true;
});
}

const testRadices = [2, 8, 10, 16, 22];
context('NaN with Payload', function () {
const NanPayloadBuffer = Buffer.from('120000000000F87F', 'hex');
const NanPayloadDV = new DataView(
NanPayloadBuffer.buffer,
NanPayloadBuffer.byteOffset,
NanPayloadBuffer.byteLength
);
const NanPayloadDouble = NanPayloadDV.getFloat64(0, true);
const serializedNanPayloadDouble = BSON.serialize({ d: NanPayloadDouble });

for (const radix of testRadices) {
it(`should support radix argument: ${radix}`, () => {
const testNumber = Math.random() * Number.MAX_VALUE;
const double = new Double(testNumber);
expect(double.toString(radix)).to.equal(testNumber.toString(radix));
});
}
it('should keep payload in serialize-deserialize roundtrip', function () {
expect(serializedNanPayloadDouble.subarray(7, 15)).to.deep.equal(NanPayloadBuffer);
});

it('should preserve NaN value in serialize-deserialize roundtrip', function () {
const { d: newVal } = BSON.deserialize(serializedNanPayloadDouble, { promoteValues: true });
expect(newVal).to.be.NaN;
});
});

it('NODE-4335: does not preserve -0 in serialize-deserialize roundtrip if JS number is used', function () {
// TODO (NODE-4335): -0 should be serialized as double
// This test is demonstrating the behavior of -0 being serialized as an int32 something we do NOT want to unintentionally change, but may want to change in the future, which the above ticket serves to track.
const value = -0;
const serializedDouble = BSON.serialize({ d: value });
const type = serializedDouble[4];
expect(type).to.not.equal(BSON.BSON_DATA_NUMBER);
expect(type).to.equal(BSON.BSON_DATA_INT);
});
});