Optimize blobstore read syscalls#10975
Conversation
…gcap#5078) Signed-off-by: yy <736262857@qq.com>
…ield merge optimization (pingcap#5078) Signed-off-by: yy <736262857@qq.com>
ti-chi-bot
Bot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
contribution
|
Hi @yy782. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
📝 WalkthroughWalkthroughBlobStore field reads now merge consecutive field indices into contiguous ranges, issuing one read per range while preserving per-field offset and checksum handling. GTest-only read-count instrumentation and coverage validate continuous, hole-containing, and scattered selections. ChangesBatch Contiguous Field Read
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant BlobStoreRead as BlobStore::read
participant BlobStorage
Caller->>BlobStoreRead: read(FieldReadInfos)
BlobStoreRead->>BlobStoreRead: group consecutive field indices
loop for each contiguous range
BlobStoreRead->>BlobStorage: read(range byte span)
BlobStorage-->>BlobStoreRead: range buffer
BlobStoreRead->>BlobStoreRead: process offsets and checksums
end
BlobStoreRead-->>Caller: page_map
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp (2)
1956-1956: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove debug
std::coutstatements from committed test.These print statements appear to be leftover debugging output (matching the PR's manual verification screenshots) rather than part of the automated test assertions. They add noise to CI test output on every run.
🧹 Proposed cleanup
- std::cout << "=== Test 1: Continuous fields {0,1,2,3,4} ===" << std::endl; { BlobStore::FieldReadInfos read_infos; read_infos.emplace_back(BlobStore::FieldReadInfo(buildV3Id(TEST_NAMESPACE_ID, page_id), entry, {0, 1, 2, 3, 4})); auto page_map = blob_store.read(read_infos); Page page = page_map.at(page_id); ASSERT_EQ(page.fieldSize(), 5); } - std::cout << "=== Test 2: Fields with hole {0,1,3,4} ===" << std::endl; { ... } - std::cout << "=== Test 3: Scattered fields {0,2,4} ===" << std::endl; { ... }Also applies to: 1965-1965, 1974-1974
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp` at line 1956, Remove the leftover debug std::cout prints from the gtest_blob_store test so the committed automated test no longer emits manual-verification noise in CI. Update the relevant test cases in gtest_blob_store by deleting the test banner output statements while keeping the actual assertions and behavior intact; use the surrounding test blocks for the continuous-fields scenarios to locate and clean all matching debug prints.
1927-1983: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftTest validates correctness but not the actual optimization (syscall reduction).
The new test only asserts
page.fieldSize(), which would pass identically with the old per-field-read implementation. It doesn't verify that contiguous fields are actually merged into fewerread()calls — the core claim of issue#5078and this PR. The PR description mentions manually countingread()calls via debug output (matching thestd::coutlines here), but that verification isn't captured as an automated assertion.Consider adding a way to count actual read syscalls per test case (e.g., via a mock/counting
FileProviderorBlobFile) and asserting on it, so a regression back to per-field reads would be caught automatically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp` around lines 1927 - 1983, The new BlobStoreTest::ReadByFieldReadInfosContinuousFields only checks page.fieldSize(), so it does not prove the optimization in BlobStore::read actually reduces underlying read calls. Add an automated syscall/read-count assertion for each FieldReadInfos case by using a counting/mock FileProvider or BlobFile around the existing BlobStore setup, and verify the contiguous-field case triggers fewer reads than the scattered/hole cases. Keep the current data setup and page assertions, but make the test fail if read() reverts to per-field behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp`:
- Line 1956: Remove the leftover debug std::cout prints from the
gtest_blob_store test so the committed automated test no longer emits
manual-verification noise in CI. Update the relevant test cases in
gtest_blob_store by deleting the test banner output statements while keeping the
actual assertions and behavior intact; use the surrounding test blocks for the
continuous-fields scenarios to locate and clean all matching debug prints.
- Around line 1927-1983: The new
BlobStoreTest::ReadByFieldReadInfosContinuousFields only checks
page.fieldSize(), so it does not prove the optimization in BlobStore::read
actually reduces underlying read calls. Add an automated syscall/read-count
assertion for each FieldReadInfos case by using a counting/mock FileProvider or
BlobFile around the existing BlobStore setup, and verify the contiguous-field
case triggers fewer reads than the scattered/hole cases. Keep the current data
setup and page assertions, but make the test fail if read() reverts to per-field
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5bb46e4a-6d65-4316-a1f6-24bde4ca9b93
📒 Files selected for processing (2)
dbms/src/Storages/Page/V3/BlobStore.cppdbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp
…on (pingcap#5078) Signed-off-by: yy <736262857@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dbms/src/Storages/Page/V3/BlobStore.cpp (1)
946-980: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead variable
field_offset_in_buffer.It is initialized and incremented but never read;
read_size_this_entryandfield_offset_in_entryalready cover the offset bookkeeping. Consider removing it.♻️ Proposed cleanup
- size_t field_offset_in_buffer = 0; for (size_t i = start_field_index; i <= end_field_index; ++i) { @@ read_size_this_entry += field_size; - field_offset_in_buffer += field_size; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/Page/V3/BlobStore.cpp` around lines 946 - 980, Remove the unused field_offset_in_buffer variable and its increment from the field iteration in the relevant BlobStore read logic; retain read_size_this_entry and field_offset_in_entry, which provide the required offset bookkeeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Storages/Page/V3/BlobStore.h`:
- Around line 183-185: Initialize the DBMS_PUBLIC_GTEST-only member
field_read_call_count at its declaration with a zero default value, ensuring
BlobStore::read(FieldReadInfos&) can safely increment it before
resetFieldReadCallCount() is called.
---
Nitpick comments:
In `@dbms/src/Storages/Page/V3/BlobStore.cpp`:
- Around line 946-980: Remove the unused field_offset_in_buffer variable and its
increment from the field iteration in the relevant BlobStore read logic; retain
read_size_this_entry and field_offset_in_entry, which provide the required
offset bookkeeping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f09b393d-d300-4dcd-9b70-2122525478bd
📒 Files selected for processing (3)
dbms/src/Storages/Page/V3/BlobStore.cppdbms/src/Storages/Page/V3/BlobStore.hdbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp
… field_offset_in_buffer Signed-off-by: yy <736262857@qq.com>
1 participant
What problem does this PR solve?
Issue Number: close #5078
Problem Summary: Optimize field-level read in BlobStore by merging continuous fields into single system call to reduce IOPS and improve read performance.
What is changed and how it works?
storage/v3: merge continuous field reads into single system call (#5078)
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests