He-Pin commented on code in PR #3526:
URL: https://github.com/apache/pekko/pull/3526#discussion_r3947120408
##########
actor/src/main/scala/org/apache/pekko/util/ByteString.scala:
##########
@@ -1581,12 +1581,27 @@ object ByteString {
/**
* Scans for the fragment containing `offset` and records it as the hint.
`hintIdx` and
* `hintStart` are a previously read hint that missed, used to resume the
scan from that
- * fragment rather than from the start.
+ * fragment -- forward or backward, whichever side of it `offset` is on --
rather than
+ * from the start.
*/
private def resolveFragment(offset: Int, hintIdx: Int, hintStart: Int):
Long = {
var pos = 0
var seen = 0
if (hintIdx >= 0) {
+ if (offset < hintStart) {
+ // moving backward before the remembered fragment: walk back from
it. `offset >= 0`
+ // and fragment 0 starts at 0, so the walk stops at fragment 0 at
the latest, and it
+ // is never longer than the scan from fragment 0 it replaces.
Review Comment:
`never longer than the scan from fragment 0 it replaces` isn't true. With
the hint at fragment N-1 (say, after a forward pass to the end) and the next
call being `apply(0)`, this walk does N-1 steps while the from-zero scan it
replaces does 0. It is still bounded by N steps per call, so nothing is
asymptotically worse — but the comment asserts an invariant that does not hold,
and that assertion is what justifies skipping a distance heuristic.
##########
actor/src/main/scala/org/apache/pekko/util/ByteString.scala:
##########
@@ -1581,12 +1581,27 @@ object ByteString {
/**
* Scans for the fragment containing `offset` and records it as the hint.
`hintIdx` and
* `hintStart` are a previously read hint that missed, used to resume the
scan from that
- * fragment rather than from the start.
+ * fragment -- forward or backward, whichever side of it `offset` is on --
rather than
+ * from the start.
*/
private def resolveFragment(offset: Int, hintIdx: Int, hintStart: Int):
Long = {
var pos = 0
var seen = 0
if (hintIdx >= 0) {
+ if (offset < hintStart) {
+ // moving backward before the remembered fragment: walk back from
it. `offset >= 0`
+ // and fragment 0 starts at 0, so the walk stops at fragment 0 at
the latest, and it
+ // is never longer than the scan from fragment 0 it replaces.
+ pos = hintIdx
+ seen = hintStart
+ while (offset < seen) {
Review Comment:
The loop has no lower bound on `pos`. `fragmentHint` is a non-volatile
`Long`, and JLS 17.7 allows a non-volatile 64-bit field to be read as two
separate 32-bit reads, so `hintIdx` and `hintStart` can come from different
writes despite the packing. The forward path merely rescans in that case; this
one drives `pos` below 0 and `bytestrings(pos)` throws
`IndexOutOfBoundsException`. `while (pos > 0 && offset < seen)` costs nothing
and degrades to a from-zero scan instead.
(The torn-read hazard itself predates this PR — the fast path in
`byteAtUnchecked` would already return a wrong byte from an inconsistent pair.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
