Skip to content

Address tiered range review comments - #6411

Open
Nithin Jose (nijosmsft) wants to merge 2 commits into
nijosmsft/range_tier_refactorfrom
nijosmsft/range_tier_review_comments
Open

Nithin Jose (nijosmsft) wants to merge 2 commits into
nijosmsft/range_tier_refactorfrom
nijosmsft/range_tier_review_comments

Conversation

@nijosmsft

Copy link
Copy Markdown
Contributor

Description

Addresses focused review feedback for the tiered range implementation stacked in #6410.

  • Uses direct capacity arithmetic for tiered indexing.
  • Uses bulk moves for block-local insertions and removals.
  • Uses wider working values while retaining compact stored fields.
  • Simplifies narrowing casts and allocation comments.
  • Calls the production stream ACK handler directly from unit tests.
  • Adds contributor documentation for the final range architecture and ownership model.

Stack:

  1. Implement tiered storage for large QUIC ranges #6391 — tiered range implementation
  2. Refactor tiered and array range implementations #6410 — storage encapsulation and policy
  3. This PR — focused review cleanups and documentation

Testing

  • Debug and Release x64 Schannel builds.
  • Debug x64 kernel solution build.
  • Full Debug x64 Schannel BVT: 3,436 tests ran, 107 skipped, zero failures or controller errors.
  • Ubuntu 24.04 GCC 13.3 and Clang 18.1 incremental Debug rebuilds with quictls, LTTng logging, tests enabled, and -Werror -Wall -Wextra.
  • Generated Linux CLOG/LTTng trace sources compiled successfully.
  • CLOG regeneration produced no source changes.

Documentation

Adds docs/contrib/Range.md covering representations, invariants, promotion, demotion, ownership, lifecycle, and mutation behavior.

@codecov

codecov Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.37%. Comparing base (508e329) to head (817bbd1).

Additional details and impacted files
@@                        Coverage Diff                        @@
##           nijosmsft/range_tier_refactor    #6411      +/-   ##
=================================================================
+ Coverage                          88.23%   88.37%   +0.13%     
=================================================================
  Files                                 64       64              
  Lines                              19393    19400       +7     
=================================================================
+ Hits                               17112    17145      +33     
+ Misses                              2281     2255      -26     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved issues remain, and the reported model checks support the revised circular-block movement logic.

No open issues 🎉

What changed in this PR

This PR refines the tiered range implementation introduced in the preceding stack, simplifying storage operations and documenting the architecture without changing the intended range semantics.

Changes:

  • Replaces shift-and-mask indexing with capacity arithmetic and block-local loops with bulk moves.
  • Removes the stream ACK test shim so unit tests call the production handler directly.
  • Documents range storage, invariants, ownership, and lifecycle transitions.
File Description
src/​core/​unittest/​StreamAckTestShim.c Deletes the redundant ACK handler wrapper.
src/​core/​unittest/​RangeTest.cpp Calls the production stream ACK handler directly.
src/​core/​unittest/​CMakeLists.txt Removes the deleted shim from test sources.
src/​core/​stream.h Adds C linkage guards for C++ callers.
src/​core/​range_tiered.h Simplifies block and directory capacity constants.
src/​core/​range_tiered.c Uses capacity arithmetic, wider working values, and bulk local moves.
docs/​contrib/​Range.md Documents range architecture and ownership.

Review effort: Balanced

@nijosmsft
Nithin Jose (nijosmsft) force-pushed the nijosmsft/range_tier_review_comments branch from 7c5fb8d to 6bbb97f Compare October 23, 2026 03:17
@nijosmsft
Nithin Jose (nijosmsft) force-pushed the nijosmsft/range_tier_review_comments branch from 6bbb97f to e654d25 Compare October 23, 2026 16:26
@nijosmsft
Nithin Jose (nijosmsft) marked this pull request as ready for review October 23, 2026 20:46
@nijosmsft
Nithin Jose (nijosmsft) requested a review from a team as a code owner October 23, 2026 20:46
Comment thread src/core/range_tiered.c Resolved
Remove redundant zero-count contiguity clauses and the redundant minimum
around the active-plus-one spare-block target. Compact helper expression
alignment while preserving insertion-prefix guards and movement behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants