Skip to content
This repository has been archived by the owner on Oct 10, 2019. It is now read-only.

Document the motivation for no mixed type operations #115

Closed
seishun opened this issue Jan 28, 2018 · 8 comments
Closed

Document the motivation for no mixed type operations #115

seishun opened this issue Jan 28, 2018 · 8 comments

Comments

@seishun
Copy link

seishun commented Jan 28, 2018

64-bit integers are often stored as two 32-bit values, see for example https://github.com/dcodeIO/long.js. If one wants to construct a BigInt from such a pair, they will have to perform a left shift by 32 bits, followed by bitwise OR. However, bitwise operations currently require both operands to be of the same type, which results in somewhat verbose code: BigInt(high) << 32n | BigInt(low).

A more specific use case is Node.js's Buffer class - I would like to add methods for reading and writing 64-bit integers once BigInt is standardized. For performance reasons, the readUInt64LE method would likely read individual bytes rather than call readUInt32LE, so it would look like this (and analogously for the other methods):

  return BigInt(this[offset]) |
      (BigInt(this[offset + 1]) << 8n) |
      (BigInt(this[offset + 2]) << 16n) |
      (BigInt(this[offset + 3]) << 24n) |
      (BigInt(this[offset + 4]) << 32n) |
      (BigInt(this[offset + 5]) << 40n) |
      (BigInt(this[offset + 6]) << 48n) |
      (BigInt(this[offset + 7]) << 56n)

The README states that mixed operands are disallowed because there is no type that would encompass both arbitrary integers and double-precision floats. But bitwise operations on Numbers convert their operands to 32-bit integers, so in this case there is a "more general" type, and that is BigInt.

@littledan
Copy link
Member

We've discussed modifications to the casting rules a few times at TC39, and the committee has so far come down pretty consistently on the side of maintaining the current rigidity, out of a sense of regularity, predictability and simplicity, even if it will make some code a little more verbose.

In this use case, is there a downside to the current BigInt semantics beyond the verbosity of the explicit casts?

@jakobkummerow
Copy link
Collaborator

If you want fewer BigInt conversions, you can change the code to this:

return BigInt(
          (this[offset]) |
          (this[offset + 1] << 8) |
          (this[offset + 2] << 16) |
          (this[offset + 3] << 24)) |
      (BigInt(
          (this[offset + 4]) |
          (this[offset + 5] << 8) |
          (this[offset + 6] << 16) |
          (this[offset + 7] << 24)) << 32n);

Are you sure that reading byte by byte is faster than using readUint32LE? I would have guessed that the latter is faster. As an alternative, you could use buffer.buffer and then construct a BigInt64Array view onto that buffer (for 8-byte-aligned reads).

@seishun
Copy link
Author

seishun commented Jan 29, 2018

#40 is still open, so I assumed changing casting rules is still up for discussion.

In this use case, is there a downside to the current BigInt semantics beyond the verbosity of the explicit casts?

No (aside from a possible performance cost), but can't that be said about all semantics that require explicit casts? On the other hand, is there a practical downside to allowing shifts of Number by BigInt?

If you want fewer BigInt conversions, you can change the code to this:

Unfortunately this code has different behavior: it returns a negative number if the highest bit is set in this[offset + 3] or this[offset + 7].

Are you sure that reading byte by byte is faster than using readUint32LE? I would have guessed that the latter is faster.

readUint32LE itself reads byte by byte, so it would just result in two extra function calls.

@jakobkummerow
Copy link
Collaborator

Unfortunately this code has different behavior: it returns a negative number if the highest bit is set in this[offset + 3] or this[offset + 7].

Good point. So you have several options, all of which should have acceptable performance:

  • extend my previous suggestion by wrapping the inner expressions in (...) >>> 0 as the last step before passing them to the BigInt constructor
  • do 3 chunks of 3, 3, 2 bytes each
  • instead of ... | (... << 24), use the same ... + (... * 0x1000000) trick as readUint32LE does
  • simply call readUint32LE twice. If that code is performance relevant for any given app, then the optimizing compiler should inline both calls when it kicks in.

@seishun
Copy link
Author

seishun commented Jan 30, 2018

Readability aside, it looks like the performance is inversely proportional to the number of BigInt calls. So it seems it does have a performance cost, at least in the current V8 implementation.

Benchmark results
-- original code (8 BigInt calls) --
buffer="fast" noAssert="false": 1.4665249002465364
buffer="slow" noAssert="false": 1.4502353256256304
buffer="fast" noAssert="true": 1.4844313897214458
buffer="slow" noAssert="true": 1.4860420344078917

-- extend my previous suggestion by wrapping the inner expressions in (...) >>> 0 as the last step before passing them to the BigInt constructor --
buffer="fast" noAssert="false": 7.470739801663214
buffer="slow" noAssert="false": 7.498603759979892
buffer="fast" noAssert="true": 7.334962643695403
buffer="slow" noAssert="true": 7.357929582215213

-- do 3 chunks of 3, 3, 2 bytes each --
buffer="fast" noAssert="false": 4.428653284002742
buffer="slow" noAssert="false": 4.434851584520805
buffer="fast" noAssert="true": 4.503965387962818
buffer="slow" noAssert="true": 4.496075335375073

-- instead of ... | (... << 24), use the same ... + (... * 0x1000000) trick as readUint32LE does --
buffer="fast" noAssert="false": 7.482254449356279
buffer="slow" noAssert="false": 7.508798942004222
buffer="fast" noAssert="true": 7.399076814226259
buffer="slow" noAssert="true": 7.385376664697134

-- simply call readUint32LE twice --
buffer="fast" noAssert="false": 7.260879734868687
buffer="slow" noAssert="false": 7.185438616669704
buffer="fast" noAssert="true": 7.1420994681335666
buffer="slow" noAssert="true": 7.269682835296691	

But that's beside the point. The main point is that every time someone needs to compose a BigInt from multiple Numbers using bitwise operations, they will have to choose between the straightforward and verbose (and possibly slow) approach, or one of multiple alternative options that are less verbose but somewhat less readable. If mixing types were allowed for bitwise operations, there would be one obvious choice.

@littledan
Copy link
Member

I don't think current benchmarks on V8 are a good reason to change the design given #26 (comment) .

I guess bitwise operations differ from arithmetic operations in that there is always a correct answer in the domain of BigInts. However, it seems like this would not actually add expressivity, just a quick abbreviation. I don't think this justifies complicating the model of requiring the same type (except comparisons), which is hoped to extend better to operator overloading.

@seishun
Copy link
Author

seishun commented Feb 5, 2018

I don't think this justifies complicating the model of requiring the same type (except comparisons), which is hoped to extend better to operator overloading.

Could you expand on this (or link somewhere where I can read about it)? Specifically, what would be disadvantage of replacing step 7 in 4.7.1 with the following (or something to that effect)?

If Type(lnum) is different from Type(rnum), convert both lnum and rnum to BigInt.

@littledan
Copy link
Member

@seishun Looks like we're getting to the point where we need a document describing the operator overloading design that BigInt was designed for. I've discussed this design with @BrendanEich, @tschneidereit and @keithamus in some detail, in addition to alluding to it in committee presentations. I think any of us could write such a document; we just have to get around to it.

We discussed operator overloading in the most recent TC39 meeting. When the notes come out, you'll be able to see some more of the discussion there.

@littledan littledan changed the title Allow mixing types for bitwise operations Document the motivation for no mixed type operations Jan 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants