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

[seishun]

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

[bnoordhuis]

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:

constlo=/* ... */;consthi=/* ... */;returnBigInt(lo)+BigInt(hi)*2n**32n;

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

[seishun]

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

[targos]

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

[devsnek]

it's also much more easily optimized :D

[seishun]

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

[seishun]

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.

[vsemozhetbyt]

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

[seishun]

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

[seishun]

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

[devsnek]

I would link to the proposal

[vsemozhetbyt]

We already have a precedent:

node/tools/doc/type-parser.js

Line 32 in cd8f06f

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

[seishun]

Good find.

[targos]

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

[vsemozhetbyt]

Addressed in #27101

[TimothyGu]

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]

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

[seishun]

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]

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

[seishun]

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

[vsemozhetbyt]

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

	constprimitive=jsPrimitives[typeText];
	if(primitive!==undefined){
	typeUrl=`${jsDataStructuresUrl}#${primitive}_type`;

So let us just leave the second one for now.

[vsemozhetbyt]

@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]

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

1. n and i are Numbers, convert i to BigInt in the call (BigInt(i) & m).
2. n is Number, i is BigInt, use != in the loop.
3. 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.

[bnoordhuis]

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.

[seishun]

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

[bnoordhuis]

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

[seishun]

@nodejs/buffer PTAL

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

[devsnek]

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

[seishun]

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

[vsemozhetbyt]

@seishun It seems something like this will do:

returnwriteBigU_Int64LE(this,value,offset,-0x8000000000000000n,0x7fffffffffffffffn);

[seishun]

@vsemozhetbyt thanks, seems good.

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

[seishun]

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]

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.

[TimothyGu]

Some doc nits. Otherwise LGTM.

[seishun]

Nits addressed.

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

[BethGriggs]

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)