Repository navigation
WIP: ENH: Step iterator positions with integer arithmetic at buffer markers - #7111
hjmjohnson wants to merge 1 commit into
Conversation
ImageSliceConstIteratorWithIndex and ImageLinearConstIteratorWithIndex hold a raw pointer to the current pixel. At the end-of-line and end-of-slice markers of a traversal that pointer lies outside the pixel buffer, where the C++ standard does not define pointer arithmetic. The markers are never dereferenced or compared. MovePosition() steps the pointer through its integer value instead, so no pointer arithmetic crosses the buffer boundary. The loop code generated by Clang is identical and GCC adds one instruction per pixel. GoogleTests check that both iterators visit every pixel of a region once in every direction and both orders.
github-actions
Bot
added
type:Infrastructure
hjmjohnson
left a comment
hjmjohnson
left a comment
There was a problem hiding this comment.
Self-review, as requested: each comment states where the original code formed a pointer outside the pixel buffer. Source: a pointer audit over a 6x5x4 float image buffered at index (10,20,30) (strides 1, 6, 30), four region kinds, every direction pair, forward and reverse: 48 slice and 24 linear configurations, 2744 and 1368 iterator operations. Counts are events across all configurations; distances are in pixels. 'Reasoned' marks a site the audit cannot observe directly because the intermediate value is overwritten within the same call.
| /** Moves the position by `n` pixels with integer arithmetic, so that a marker past the buffer is not formed by | ||
| * pointer arithmetic; the position is dereferenced only at a pixel of the buffer. */ | ||
| void | ||
| MovePosition(const OffsetValueType n) |
There was a problem hiding this comment.
The step is done on the pointer's integer value so that no pointer arithmetic produces a value outside the buffer ([expr.add]/4). Measured against main with the slice loop: Clang (arm64 23, x86 18) folds this back to the original pointer arithmetic, same instructions per pixel; GCC 13 keeps one extra instruction per pixel (10.3 -> 11.3) at +0% wall time on long lines and +2% to +9% on 2-pixel lines.
There was a problem hiding this comment.
@gdevenyi This seems like a lot of compiled away conversion to do the same math to avoid an un-observed problem.
This solution does not have the performance degredation that your PR 7032 had.
| { | ||
| this->m_PositionIndex[m_Direction_A]++; | ||
| this->m_Position += m_PixelJump; | ||
| this->MovePosition(static_cast<OffsetValueType>(m_PixelJump)); |
There was a problem hiding this comment.
The last ++ of every line. Along a first direction with stride > 1 the marker lands one stride past the last pixel: 86 of 2744 operations, up to 29 pixels beyond one-past-the-end (direction 2 on the full region: 29 of the 30 lines). Along direction 0 on a full region it lands on the next line's first pixel or exactly one-past-the-end, which the standard allows; on a sub-region ending before the buffer end it is 1 to 3 pixels beyond.
| { | ||
| this->m_PositionIndex[m_Direction_A]--; | ||
| this->m_Position -= m_PixelJump; | ||
| this->MovePosition(-static_cast<OffsetValueType>(m_PixelJump)); |
There was a problem hiding this comment.
The last -- of every reversed line: 94 of 2744 operations put the pointer below the buffer, up to 30 pixels (direction 2 reversed). Even direction 0 on the full region reaches buffer - 1 at the first line (2 events).
| // Move to next line | ||
| this->m_PositionIndex[m_Direction_B]++; | ||
| this->m_Position += m_LineJump; | ||
| this->MovePosition(static_cast<OffsetValueType>(m_LineJump)); |
There was a problem hiding this comment.
NextLine starts from the end-of-line marker of the previous comment, so this intermediate value is computed from a pointer that is already outside the buffer (reasoned; the audit observes only the result of the whole call).
| // Move to beginning of line | ||
| this->m_PositionIndex[m_Direction_A] = this->m_BeginIndex[m_Direction_A]; | ||
| this->m_Position -= m_PixelJump * (this->m_EndIndex[m_Direction_A] - this->m_BeginIndex[m_Direction_A]); | ||
| this->MovePosition( |
There was a problem hiding this comment.
After the last line of a slice this lands on the end-of-slice marker along the second direction: 15 of the 48 configurations end NextLine with the pointer up to 27 pixels beyond one-past-the-end.
| { | ||
| this->m_PositionIndex[m_Direction]--; | ||
| this->m_Position -= m_Jump; | ||
| this->MovePosition(-m_Jump); |
There was a problem hiding this comment.
47 of 1368 operations put the pointer below the buffer, up to 30 pixels; direction 0 reaches buffer - 1 at the first line.
| { | ||
| this->m_Position -= | ||
| this->m_OffsetTable[m_Direction] * (this->m_PositionIndex[m_Direction] - this->m_BeginIndex[m_Direction]); | ||
| this->MovePosition(-this->m_OffsetTable[m_Direction] * |
There was a problem hiding this comment.
NextLine starts from the end-of-line marker, so this first step is computed from an out-of-buffer pointer whenever the previous comment applies (reasoned). The call itself always ends on a pixel or, when exhausted, at the region's begin, so the audit sees no out-of-buffer result from NextLine.
| { | ||
| this->m_Position += | ||
| this->m_OffsetTable[m_Direction] * (this->m_EndIndex[m_Direction] - 1 - this->m_PositionIndex[m_Direction]); | ||
| this->MovePosition(this->m_OffsetTable[m_Direction] * |
There was a problem hiding this comment.
Mirror for PreviousLine: starts from the reverse end-of-line marker, below the buffer after the last -- (reasoned).
| const OffsetValueType distanceToEnd = this->m_EndIndex[m_Direction] - this->m_PositionIndex[m_Direction]; | ||
|
|
||
| this->m_Position += m_Jump * distanceToEnd; | ||
| this->MovePosition(m_Jump * distanceToEnd); |
There was a problem hiding this comment.
GoToEndOfLine deliberately places the pointer one stride past the last pixel: along direction 2 from a line that does not end at the buffer end that is beyond one-past-the-end (2 events in the audit, up to 21 pixels). From the last line of the full region it is exactly one-past-the-end, which is allowed.
| const OffsetValueType distanceToEnd = this->m_EndIndex[m_Direction] - this->m_PositionIndex[m_Direction] - 1; | ||
|
|
||
| this->m_Position += m_Jump * distanceToEnd; | ||
| this->MovePosition(m_Jump * distanceToEnd); |
There was a problem hiding this comment.
GoToReverseBeginOfLine and GoToBeginOfLine (line 57) always end on a pixel; the step may start from an end-of-line marker outside the buffer when called there (reasoned).
This is to better understand the un-observed failure mode that was detected.
Supersedes #7032. The slice and linear
WithIndexiterators step their pixel pointer through its integer value at everym_Position +=/-=site, so no pointer arithmetic crosses the buffer boundary at end-of-line and end-of-slice markers. No branch is added to the per-pixel step; the generated loop is unchanged on Clang and GCC adds one instruction per pixel. WIP: may still be closed in favor of documenting the invariant instead.Why not #7032
#7032 recomputes the pointer from the image on a second path inside
operator++/operator--. That stops the compiler from keeping the iterator in registers: the per-pixel loop reloads its bound and stores the pointer every pixel. Measured on the documented slice loop over a float 3-D image, best of 7 runs: x86 GCC 13 +196% to +208% wall time on 64^3 and 256^3 images, x86 Clang 18 +141% to +242%, arm64 Clang +3% to +7% on long lines and +79% on 2-pixel lines. Forcing the helper inline or out of line, or using one range test, changes none of this.Is there an observable defect?
No. On
mainthe marker pointers lie outside the buffer in 229 of 2744 operations over all direction pairs, four region kinds and both orders (up to 60 elements away), but every traversal visits every pixel once with the right values, the markers are never dereferenced or compared, and AddressSanitizer, UndefinedBehaviorSanitizer and valgrind are silent on arm64 Clang, x86 GCC and x86 Clang. The change is for conformance with [expr.add]: the step is performed on the pointer's integer value, which is implementation-defined rather than undefined, and the pointer is dereferenced only at a pixel of the buffer.Measurements of this change
Same harness, this branch's headers against
main, instructions per pixel: arm64 Clang 9.2 vs 9.3 (64^3), identical for strided, reverse and mutable loops; x86 Clang -15%; x86 GCC +10% (one instruction). Wall time, best of 7 interleaved runs: arm64 -9% to +1%; x86 GCC -1% to +9%, the +9% on reverse 2-pixel lines; x86 Clang -11% to +6%. The pointer audit, sanitizers and the new GoogleTests pass on all three. Harness and raw results:.devlocal/reviews/pr7032/perf/in the author's checkout (findings.md).The new GoogleTests check that both iterators visit every pixel of a region exactly once in every direction and both orders, for regions at the buffer edges and in its interior; they cannot observe the pointer values and pass on
maintoo. #7032's tests asserted the address of the protectedm_Position, which this change does not constrain.AI assistance
Claude Code (Opus 5.5 / Fable 5.1) wrote the benchmark and audit harness, ran the measurements on macOS arm64 and Linux x86_64, evaluated nine alternative implementations, and drafted the code and tests. The author reviewed and is responsible for the change.