Move block parsing to preload for optimized --preload-measurement-range performance #2625
Conversation
|
CI gfxreconstruct build queued with queue ID 631199. |
jzulauf-lunarg
requested review from
MarkY-LunarG,
bradgrantham-lunarg,
fabian-lunarg and
ncesario-lunarg
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d0508e67d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| // We can succeed at EOF, if there are more files on the stack. | ||
| success = HandleBlockEof("preload", false /* no frame or block info */); | ||
| // Normal case, just add the preloaded frame | ||
| preloaded_frames_.emplace_back(std::make_unique<PreloadedFrame>(current_preload_frame)); | ||
| preloaded_frames_.back()->AppendMovedBlocks(frame_blocks); |
There was a problem hiding this comment.
This “normal case” enqueues a preloaded frame for any non-error result. When PreloadBlocksOneFrame ends with kEndProcessing (e.g., EOF right after the previous frame boundary or an early stop from block limits), frame_blocks can be empty or represent a partial frame, but it still gets queued here. During replay, ReplayOneFrame always returns kFrameBoundary, so ProcessNextFrame increments current_frame_number_ for that partial/empty frame, which doesn’t happen in the non-preload path and can shift frame counts/measurement range by one. Consider only enqueuing when preload_result == kFrameBoundary, or carry a “complete frame” flag so replay doesn’t increment for partial EOF frames.
Useful? React with 👍 / 👎.
|
CI gfxreconstruct build # 8690 running. |
|
CI gfxreconstruct build # 8690 passed. |
jzulauf-lunarg
force-pushed
the
zulauf-parsed-preload
branch
from
6d0508e to
f4e9161
Compare
|
CI gfxreconstruct build queued with queue ID 634393. |
|
CI gfxreconstruct build # 8728 running. |
|
CI gfxreconstruct build # 8728 passed. |
jzulauf-lunarg
force-pushed
the
zulauf-parsed-preload
branch
from
f4e9161 to
303b5a4
Compare
|
CI gfxreconstruct build queued with queue ID 634664. |
|
CI gfxreconstruct build # 8731 running. |
|
CI gfxreconstruct build # 8731 passed. |
ncesario-lunarg
left a comment
ncesario-lunarg
left a comment
There was a problem hiding this comment.
LGTM pending others' reviews.
| bool IsReady() const { return state_ == BlockState::kReady; } | ||
| bool IsVisitable() const { return (state_ == BlockState::kReady) || (state_ == BlockState::kDeferredDecompress); } | ||
| bool IsUnknown() const { return state_ == BlockState::kUnknown; } | ||
| bool IsValid() const noexcept { return state_ != BlockState::kInvalid; } |
There was a problem hiding this comment.
Does gfxreconstruct use exceptions?
There was a problem hiding this comment.
It's there to give compiler the hint that it doesn't need to at guard logic...
| // GFXRECON_LOG_INFO("Frame delimiter encountered during preload, ending frame preload."); | ||
| break; | ||
| } | ||
| // This is really part of the non-preloaed previous (first) frame, |
There was a problem hiding this comment.
nit: "preloaed" -> "preload"
Replay storage is vector of ParsedBlocks moved from ad deque of parsed blocks Also fixes corner cases in 1st, 2nd, and stutter frames, and extra frame when --quit-after-measurment range.
jzulauf-lunarg
force-pushed
the
zulauf-parsed-preload
branch
from
303b5a4 to
c6342fc
Compare
|
CI gfxreconstruct build queued with queue ID 635662. |
|
CI gfxreconstruct build # 8743 running. |
|
CI gfxreconstruct build # 8743 passed. |
| // We assume that only known, vistable blocks were preloaded | ||
| GFXRECON_ASSERT(queued_block.IsVisitable()); | ||
|
|
||
| bool decompressed = false; |
|
|
||
| SetDecoderBlockIndex(block_index); | ||
| std::visit(dispatch_visitor, queued_block.GetArgs()); | ||
| block_index++; |
| // Block processing will update current_frame_number_, so save and restore it, | ||
| // as callers rely on it remaining unchanged by preload. | ||
| const uint64_t save_current_frame = current_frame_number_; | ||
| bool success = true; |
| }; | ||
|
|
||
| public: | ||
| const static uint32_t kFirstFrame = 0; |
There was a problem hiding this comment.
This can be constexpr uint64_t
4 participants
Preload storage is vector of PreloadFrames containing vectors of ParsedBlocks moved from a deque of parsed blocks created during pre-loading. Data construction and teardown are outside the measurement window.
Also fixes corner cases in 1st, 2nd, and stutter frames (when ProcessBlocks returns the same frame number twice), and removes extra frame when --quit-after-measurment-range is set.
NOTE: This is currently built on Performance Clawback II so it' has the "zulauf-unbuffered-read" change commit, so for clarity only review the changes of the last commit.