buffer: add {read|write}Big[U]Int64{BE|LE} methods #19691

seishun · 2018-03-30T09:20:43Z

This is a resurrection of #15152, this time with BigInts.

The functions writeBigU_Int64LE and writeBigU_Int64BE look so terrible because setting Uint8Array values to BigInt is disallowed by the spec. I submitted an issue about it in tc39/proposal-bigint#137. Please 👍 it or whatever if you agree.

Checklist

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

doc/api/buffer.md

bnoordhuis · 2018-03-30T12:29:38Z

lib/internal/buffer.js

+  if (first === undefined || last === undefined)
+    boundsError(offset, this.length - 8);
+
+  // TODO: do we need tricky math from readUInt48LE?


You mean the (a + b * 256) * 2 ** 32 bit?

I expect it's faster to compute the high and low words as 32 bits quantities first, then convert them to bigints:

const lo = /* ... */; const hi = /* ... */; return BigInt(lo) + BigInt(hi) * 2n ** 32n;

I'm unsure if V8 currently constant-folds the 2n ** 32n so you might want to cache that.

It's probably faster now, but I would prefer to put off any optimizations until V8 6.7 is out.

Have you considered using A << Bn instead of A * 2n ** Bn (everywhere)?
It's shorter and IMO easier to read.

it's also much more easily optimized :D

I chose to use the same approach as in other read functions.

Caching is indeed faster:

confidence improvement accuracy (*) (**) (***) buffers\\buffer-read.js n=1000000 type='BigInt64BE' buffer='fast' *** 36.14 % Â±4.71% Â±6.26% Â±8.15% buffers\\buffer-read.js n=1000000 type='BigInt64BE' buffer='slow' *** 32.26 % Â±5.68% Â±7.58% Â±9.92% buffers\\buffer-read.js n=1000000 type='BigInt64LE' buffer='fast' *** 37.72 % Â±4.18% Â±5.58% Â±7.29% buffers\\buffer-read.js n=1000000 type='BigInt64LE' buffer='slow' *** 34.75 % Â±5.00% Â±6.66% Â±8.67% buffers\\buffer-read.js n=1000000 type='BigUInt64BE' buffer='fast' *** 34.65 % Â±3.41% Â±4.56% Â±5.97% buffers\\buffer-read.js n=1000000 type='BigUInt64BE' buffer='slow' *** 35.73 % Â±3.70% Â±4.92% Â±6.42% buffers\\buffer-read.js n=1000000 type='BigUInt64LE' buffer='fast' *** 30.61 % Â±3.91% Â±5.20% Â±6.78% buffers\\buffer-read.js n=1000000 type='BigUInt64LE' buffer='slow' *** 32.13 % Â±3.36% Â±4.47% Â±5.83%

But using bitwise shift instead of multiplication as @targos suggested is equally fast, and less ugly than a constant, so I went with that.

doc/api/buffer.md

lib/internal/buffer.js

test/parallel/test-buffer-bigint64.js

vsemozhetbyt · 2018-03-30T15:54:14Z

doc/api/buffer.md

+-->
+
+* `offset` {integer} Number of bytes to skip before starting to read. Must satisfy: `0 <= offset <= buf.length - 8`.
+* Returns: {bigint}


It seems MDN have not a BigInt doc yet, but maybe we can add a link to spec or proposal in customTypesMap for now?

Let's put it off for now. Maybe it will be on MDN when it's time to land.

Still not on MDN. What do others think about adding a link to the proposal?

I would link to the proposal

We already have a precedent:

node/tools/doc/type-parser.js

Line 32 in cd8f06f

'AsyncIterator': 'https://github.com/tc39/proposal-async-iteration',

It is on MDN now: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

Addressed in #27101

doc/api/buffer.md

TimothyGu · 2018-03-30T17:00:07Z

Have you considered using something that already exists in the language, namely DataView#getBigInt64 and friends? The complicated logic in our functions makes it seem worthwhile. Maybe by doing so, we can finally encourage the V8 team to optimize DataView more 😜 Cf. #2897

BridgeAR · 2018-04-09T16:48:05Z

@TimothyGu I doubt that this is going to be as fast as doing this in JS but we could of course try.

seishun · 2018-06-03T17:33:48Z

Comments addressed, PTAL.

@TimothyGu last time this was attempted (in the PR you linked) the results were disappointing, so I don't think it would be worthwhile to pursue this again.

seishun · 2018-06-03T17:41:00Z

Added a link to the spec. Hope it works, because I can't build docs on Windows.

seishun · 2018-06-03T17:42:23Z

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

vsemozhetbyt · 2018-06-03T17:46:48Z

tools/doc/type-parser.js

@@ -4,6 +4,7 @@ const jsDocPrefix = 'https://developer.mozilla.org/en-US/docs/Web/JavaScript/';

 const jsDataStructuresUrl = `${jsDocPrefix}Data_structures`;
 const jsPrimitives = {
+  bigint: 'BigInt',


Unfortunately, we cannot add this here, otherwise, the link would be wrong from this code path:

node/tools/doc/type-parser.js

Lines 140 to 143 in cd8f06f

const primitive = jsPrimitives[typeText];

if (primitive !== undefined) {

typeUrl = `${jsDataStructuresUrl}#${primitive}_type`;

So let us just leave the second one for now.

tools/doc/type-parser.js

vsemozhetbyt · 2018-06-03T17:56:32Z

@seishun

Hope it works, because I can't build docs on Windows.

It is possible, though tricky.

Building just one doc is simpler, but it will not be styled properly if not placed with assets (which is not needed for simple build test:

node.exe tools/doc/generate.js --format=html --node-version=11.0.0 --analytics= doc/api/buffer.md > buffer.html

seishun · 2018-06-05T09:34:45Z

benchmark/buffers/buffer-write.js

+  for (var i = 0n; i !== n; i++) {
+    buff[fn](i & m, 0);
+  }
+  bench.end(Number(n));


It's minor, but I had trouble choosing between the following options:

n and i are Numbers, convert i to BigInt in the call (BigInt(i) & m).

n is Number, i is BigInt, use != in the loop.

n and i are BigInts, convert to Number before bench.end() call.

It's now using option 3, but I'm not sure it's the best.

Seems fine to me. You could do const nn = Number(n) before bench.start() if you're worried about the coercion influencing the benchmark numbers.

It's already converted to BigInt in main().

I mean the coercion in bench.end(Number(n)).

seishun · 2018-06-09T14:46:47Z

@nodejs/buffer PTAL

I'm not sure how to fix the message: '''BigInt'' is not defined.' linter errors.

devsnek · 2018-06-09T14:58:29Z

@seishun add BigInt to the globals object in .eslintrc.js https://github.com/nodejs/node/blob/master/.eslintrc.js#L245

seishun · 2018-06-09T16:22:21Z

The remaining issue is lines 594 and 598. How do we usually deal with long function calls?

vsemozhetbyt · 2018-06-09T16:32:21Z

@seishun It seems something like this will do:

  return writeBigU_Int64LE(
    this, value, offset, -0x8000000000000000n, 0x7fffffffffffffffn);

seishun · 2018-06-09T19:22:03Z

@vsemozhetbyt thanks, seems good.

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

seishun · 2018-06-11T07:00:53Z

Rebased on master now that #21237 has landed.

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

bnoordhuis

LGTM with comments/suggestions.

bnoordhuis · 2018-06-11T08:51:07Z

benchmark/buffers/buffer-write.js

+  for (var i = 0n; i !== n; i++) {
+    buff[fn](i & m, 0);
+  }
+  bench.end(Number(n));


Seems fine to me. You could do const nn = Number(n) before bench.start() if you're worried about the coercion influencing the benchmark numbers.

doc/api/buffer.md

lib/internal/buffer.js

test/parallel/test-buffer-bigint64.js

TimothyGu

Some doc nits. Otherwise LGTM.

doc/api/buffer.md

seishun · 2018-06-13T14:46:12Z

Nits addressed.

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

doc/api/buffer.md

PR-URL: nodejs/node#19691 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

BethGriggs · 2019-09-03T15:40:13Z

I've added the backport-requested-v10.x label as this change doesn't land cleanly on v10.x. If you feel this change should land please open a backport PR (backporting guide)

Backport-PR-URL: nodejs#30361 PR-URL: nodejs#19691 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> (cherry picked from commit 3d8532f)

Backport-PR-URL: #30361 PR-URL: #19691 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - upgrade npm to 6.13.7 (Michael Perrotte) [#31558](#31558) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) PR-URL: #31984

Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - upgrade npm to 6.13.7 (Michael Perrotte) [#31558](#31558) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) PR-URL: #31984

Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) PR-URL: #31984

Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984

Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984

Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984

macOS package notarization and a change in builder configuration The macOS binaries for this release, and future 10.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing .pkg files on macOS 10.15 and later. Previous builds of Node.js 10.x were compiled on macOS 10.7 (Lion). As binaries are still being compiled to support a minimum of macOS 10.7 (Lion) we do not anticipate this having a negative impact on Node.js 10.x users with older versions of macOS. Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984

macOS package notarization and a change in builder configuration The macOS binaries for this release, and future 10.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing .pkg files on macOS 10.15 and later. Previous builds of Node.js 10.x were compiled on macOS 10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion). As binaries are still being compiled to support a minimum of macOS 10.7 (Lion) we do not anticipate this having a negative impact on Node.js 10.x users with older versions of macOS. Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984

seishun requested a review from BridgeAR March 30, 2018 09:20

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Mar 30, 2018

mscdex added the blocked PRs that are blocked by other issues or PRs. label Mar 30, 2018

bnoordhuis reviewed Mar 30, 2018

View reviewed changes

BridgeAR reviewed Mar 30, 2018

View reviewed changes

doc/api/buffer.md Outdated Show resolved Hide resolved

doc/api/buffer.md Outdated Show resolved Hide resolved

doc/api/buffer.md Outdated Show resolved Hide resolved

lib/internal/buffer.js Show resolved Hide resolved

test/parallel/test-buffer-bigint64.js Outdated Show resolved Hide resolved

vsemozhetbyt reviewed Mar 30, 2018

View reviewed changes

devsnek removed the blocked PRs that are blocked by other issues or PRs. label Jun 3, 2018

seishun changed the title ~~[WIP] buffer: add {read|write}Big[U]Int64{BE|LE} methods~~ buffer: add {read|write}Big[U]Int64{BE|LE} methods Jun 3, 2018

vsemozhetbyt reviewed Jun 3, 2018

View reviewed changes

tools/doc/type-parser.js Outdated Show resolved Hide resolved

seishun commented Jun 5, 2018

View reviewed changes

seishun mentioned this pull request Jun 9, 2018

tools: add BigInt to globals #21237

Closed

2 tasks

bnoordhuis approved these changes Jun 11, 2018

View reviewed changes

TimothyGu approved these changes Jun 13, 2018

View reviewed changes

doc/api/buffer.md Outdated Show resolved Hide resolved

joyeecheung approved these changes Jun 13, 2018

View reviewed changes

TimothyGu approved these changes Jun 13, 2018

View reviewed changes

vsemozhetbyt reviewed Jun 14, 2018

View reviewed changes

doc/api/buffer.md Outdated Show resolved Hide resolved

Flarna mentioned this pull request Apr 24, 2019

feat(node): v12 DefinitelyTyped/DefinitelyTyped#34952

Merged

9 tasks

BethGriggs added the lts-watch-v10.x label Aug 20, 2019

BethGriggs added the backport-requested-v10.x label Sep 3, 2019

fanatid mentioned this pull request Sep 17, 2019

readBigUInt64BE() and similar methods are missing feross/buffer#244

Closed

GaryGSC mentioned this pull request Nov 11, 2019

[v10.x backport] buffer: add {read|write}Big[U]Int64{BE|LE} methods #30361

Closed

4 tasks

BethGriggs mentioned this pull request Mar 12, 2020

v10.20.0 proposal #31984

Merged

BethGriggs added land-on-v10.x and removed backport-requested-v10.x labels Jul 1, 2020

	const primitive = jsPrimitives[typeText];

	if (primitive !== undefined) {
	typeUrl = `${jsDataStructuresUrl}#${primitive}_type`;

buffer: add {read|write}Big[U]Int64{BE|LE} methods #19691

buffer: add {read|write}Big[U]Int64{BE|LE} methods #19691

Conversation

seishun commented Mar 30, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 30, 2018

BridgeAR commented Apr 9, 2018

seishun commented Jun 3, 2018

seishun commented Jun 3, 2018

seishun commented Jun 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun commented Jun 9, 2018

devsnek commented Jun 9, 2018

seishun commented Jun 9, 2018

vsemozhetbyt commented Jun 9, 2018

seishun commented Jun 9, 2018

seishun commented Jun 11, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

seishun commented Jun 13, 2018 • edited Loading

BethGriggs commented Sep 3, 2019

seishun commented Mar 30, 2018 •

edited

Loading

seishun commented Jun 13, 2018 •

edited

Loading