VarHandle multi-byte reads for ByteString lastIndexOf#2838
VarHandle multi-byte reads for ByteString lastIndexOf#2838pjfanning wants to merge 2 commits intoapache:mainfrom
Conversation
He-Pin
left a comment
There was a problem hiding this comment.
Good performance work, but a few concerns:
Concerns:
-
ByteString1C.lastIndexOfandByteString1.lastIndexOf-- The SWAR-based specializedlastIndexOf(elem: Byte, end: Int)looks correct but theunrolledLastIndexOftail handling is complex. The tests cover basic cases but I'd recommend adding a test that specifically targets the boundary between tail and full 8-byte chunks (e.g. length=15, element at position 14). -
SWARUtil.getLastIndex-- the doc saysCurrently only supports big endian. However it's used by thelastIndexOfimplementation which reads viaSWARUtil.getLong(bytes, chunkStart)(the default big-endian path). This is correct, but worth explicitly documenting to prevent future misuse. -
The PR description says WIP and needs benchmarking. I'd recommend not merging until benchmarks confirm the SWAR
lastIndexOfis actually faster than the simple loop for typical ByteString sizes (many ByteStrings are small, where SWAR overhead may not pay off). -
ByteIteratoroverrides -- usingbyteOrder == ByteOrder.BIG_ENDIANboolean flag is cleaner than the if/else chain in #2847. Consider whether these two PRs should be coordinated to avoid duplicate implementations.
|
I'm breaking this up into smaller PRs |
… SWAR lastIndexOf Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/1a4cc51a-e270-46cd-9cf0-e59a0e608650 Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com> perf: add clarifying comments to unrolledLastIndexOf methods Agent-Logs-Url: https://github.com/pjfanning/incubator-pekko/sessions/1a4cc51a-e270-46cd-9cf0-e59a0e608650 Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com> lastIndexOf (specialized) scalafmt Update SWARUtil.scala Update ByteString.scala Update ByteString.scala
0be3a08 to
21cfe12
Compare
|
I removed the parts of the original PR and focused on SWAR lastIndexOf. The other parts were merged already in other PRs. |
SWARUtil — extended VarHandle infrastructure
byteArrayViewVarHandleinstances forshort(BE+LE),int(BE+LE),long(LE)getLastIndex(word)— finds rightmost byte match in SWAR result usingnumberOfTrailingZerosByteString
Significant improvement in benchmark.