Implement Buffer swap16, swap32, swap64 #1659
Conversation
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Thank you for this. It's great that you wrote tests.
Some comments.
To make this fast, we could probably copy some of the code from node-bswap (it has an MIT license). Would need to verify that the output is the same though
| return JSC::JSValue::encode(jsUndefined()); | ||
| } | ||
|
|
||
| uint8_t* typedVector = castedThis->typedVector(); |
There was a problem hiding this comment.
Typed Arrays can become "detached". When detached, the vector or typedVector will return a nullptr. That means we have to check for this. It's fortunately pretty uncommon, so we can mark the branch as UNLIKELY
if (UNLIKELY(castedThis->isDetached())) {
throwVMTypeError(lexicalGlobalObject, throwScope, "Buffer is detached"_s);
return JSValue::encode(jsUndefined());
}| return JSC::JSValue::encode(jsUndefined()); | ||
| } | ||
|
|
||
| uint8_t* typedVector = castedThis->typedVector(); |
There was a problem hiding this comment.
need to check for isDetached
src/bun.js/bindings/JSBuffer.cpp
Outdated
| uint8_t* typedVector = castedThis->typedVector(); | ||
|
|
||
| for (size_t i = 0; i < length/size; i++) { | ||
| for (size_t j = 0; j < size/2; j++) { |
There was a problem hiding this comment.
- Ideally, this would use SIMD, leveraging AVX2 on x64 and ARM NEON on aarch64.
- We can move
size/2out of the inner loop
src/bun.js/bindings/JSBuffer.cpp
Outdated
|
|
||
| for (size_t i = 0; i < length/size; i++) { | ||
| for (size_t j = 0; j < size/2; j++) { | ||
| uint8_t temp = typedVector[size*i+j]; |
There was a problem hiding this comment.
This is a lot of multiplication. Can we turn this into one add in the outer loop and one add in the inner loop?
| ["", ""], | ||
| ["a1", "1a"], | ||
| ["a1b2", "1a2b"] | ||
| ]; |
There was a problem hiding this comment.
Can we also copy node's tests for this? https://github.com/nodejs/node/blob/master/test/parallel/test-buffer-swap.js#L1-L153
- Use constexpr - Clean up the indexing - Check for detached - Use suggested text for exception text
|
We need this and I don't think it's worth packages not working by this not being implemented So I'm going to merge this. Thank you |
2 participants
The
Bufferapi is not complete as detailed in #155.Specifically
swap16,swap32andswap64are unimplemented.This PR:
swap16,swap32andswap64insrc/bun.js/bindings/JSBuffer.cpptest/bun.js/buffer.test.js, testing:swap16,swap32andswap64respectively)To do: