-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
Unaligned bit arrays on the JavaScript target #3946
base: main
Are you sure you want to change the base?
Unaligned bit arrays on the JavaScript target #3946
Conversation
2496b81
to
bb69dd9
Compare
b347126
to
9aa5446
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, what a fantastic bit of work! Thank you!
I've not digested these changes properly yet, but I have some initial questions from my first review.
There's a lot of public functions in the API of the bit array class now, but generally the aim is to have none. What is the motivation for having these?
Similar for the deprecated functions, we can remove them.
There's quite a lot of code in the class. Could we move them to free-standing functions and have the generated code only use them if absolutely necessary? That would help with JavaScript bundlers performing dead code elimination.
Some existing tests have been removed or changed, why is this? New features shouldn't alter existing tests, it makes it harder to review and to prevent regressions.
Thanks again!
544c327
to
0f6fbdb
Compare
Thanks for taking a look!
These have been reduced down to the following:
I've removed all except the
Done.
The diff on the tests looked a bit more complex in places than it actually was so I've moved some things around to make it easier to digest. Some existing tests do need to be tweaked or removed, e.g. those that were testing for compilation errors if unaligned bit arrays were used on the JS target. |
0f6fbdb
to
25d0be4
Compare
Also, if you could weigh in on the question at the end of the initial writeup about whether we should make bit array slices O(1) in all cases that would be helpful, because if that's a yes then more work is needed prior to being ready for final review. |
25d0be4
to
f7034b6
Compare
Summary 📘
This PR adds support for unaligned bit arrays on the JavaScript target. Specifically:
In expressions:
bits
segments:In patterns:
int
segments:bits
segments:There is a warning if the above features are used when
gleam.toml
specifies a version < v1.7.0.Implementation Details 🛠️
BitArray
class in the prelude now has bothbitSize
andbyteSize
fields.BitArray
class in the prelude has been reworked in a few ways:get rawBuffer()
,get bitSize()
,get byteSize()
,get isWholeBytes()
,byteAt()
.get buffer()
,get length()
. Using these emits a deprecation warning at runtime.JSDoc
annotations have been added to all functions allowing type-checking by adding// @ts-check
to the top of the file.BitArray.sliceToInt()
has internal variants for aligned and unaligned access, as well as variants for bothnumber
andBigInt
. Thenumber
variant is used when the size is <= 53 bits.BigInt
is typically 5-10x slower, hence the decision to support both paths.Implications for
@external
JavaScript code 🌍BitArray.length
andBitArray.buffer
APIs.bitSize
up to a multiple of 8, and operate on the undefined low bits in the final byte, which will probably lead to the wrong output.Implications for
gleam/stdlib
🤝gleam/stdlib
ready to go, mostly affectinggleam/bit_array
. It can only be merged once this PR goes in as its tests don't run on Gleam 1.6.3. It may be necessary to run the new stdlib tests on nightly for a short period, with them segregated into their own file so they can be included/excluded depending on the active Gleam version. I'll sort that out once this PR makes it through review.Testing 🧪
There's certainly some complexity and tricky bitwise operations here, mostly in the JavaScript prelude. The following has been done to ensure correctness:
language_tests.gleam
, andtest/javascript_prelude
.BitArray.slice()
,BitArray.sliceToInt()
,BitArray.sliceToFloat()
is covered by at least one test.Limitations 🤔
The main limitation is that there is no allowance for unused high bits in the first byte of a bit array's buffer.
The motivation for allowing this would be to make bit array slices O(1) in all cases. Currently a slice is O(1) only if its start offset is byte-aligned (the end offset doesn't matter). If the start offset isn't byte-aligned then a slice is O(N) due to requiring a copy.
This makes the following O(N²) on JavaScript, but O(N) on Erlang:
This could be addressed at a later date, albeit with another round of impact on JavaScript FFI code that would need updating. So maybe it's better to bite the bullet now? Or maybe it's not important enough to warrant the additional complexity. There's also a reasonably good chance that any folks affected by this would be able to rework their code to avoid the performance issue (if they realise what the problem is).
✨✨✨