Skip to content

Conversation

samyron
Copy link
Contributor

@samyron samyron commented Sep 19, 2025

After thinking about #859 and the fix #860 all day, something didn't feel quite right. After doing some debugging tonight, I realized the fix might defeat some of the SWAR optimizations.

When creating the ByteBuffer like so ByteBuffer.wrap(ptrBytes, 0, len), we are looking at a different portion of the ptrBytes array when ptr > 0. While we end up reading the correct chunk with bb.getLong(ptr + pos) or bb.getInt(ptr + pos), we may end up terminating the SWAR-optimized loop early.

Consider:

irb(main):001> require "json"
irb(main):002> s = "01234567890"
irb(main):003> JSON.dump(s[2..-1])
ptr: 2
ptrBytes.length: 15
ptrBytes: [48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 48, 0, 0, 0, 0]
len: 9

The backing array has a length of 15 bytes but the length of the string is 9 bytes.

The ByteBuffer in this case is [48, 49, 50, 51, 52, 53, 54, 55, 56] because we start it at index 0. When checking if ptr + pos + 8 <= len we get 2 + 0 + 8 <= 9 which is false.

We really should be looking at [50, 51, 52, 53, 54, 55, 56, 57, 48] assuming we do not offset the bounds check by ptr, as we did prior to the fix earlier today.

There are two possible fixes:

  1. Initialize the ByteBuffer as ByteBuffer.wrap(ptrBytes, ptr, len) and remove the ptr offset in the bounds checks.
  2. Initialize the ByteBuffer as ByteBuffer.wrap(ptrBytes, 0, ptrBytes.length) and leave the bounds checks offset by ptr.

CC: @headius

Copy link
Contributor

@headius headius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either fix would be fine from a style perspective so it's just a matter of what fits the flow of the code best while un-breaking SWAR. I see you went with the first option and I think that's just fine.

@byroot byroot merged commit e809fab into ruby:master Sep 20, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants