Skip to content

refactor: prepare for lazy key loading#310

Merged
maksymar merged 11 commits intomainfrom
maksym/prep
May 21, 2025
Merged

refactor: prepare for lazy key loading#310
maksymar merged 11 commits intomainfrom
maksym/prep

Conversation

@maksymar
Copy link
Copy Markdown
Contributor

@maksymar maksymar commented May 21, 2025

This PR refactors the codebase to prepare for lazy key loading.

  • Introduces a shared LazyObject structure for both keys and values
  • Unifies the API with methods like get_key / get_value, key / value, and upcoming extract_key / extract_value

The refactoring does not change functionality, but introduces a minor performance regression.
This is due to switching from direct key reads to indexed access, aligning with the future lazy-loading model.

  instructions:
    status:   No significant changes detected 👍
    counts:   [total 304 | new 0 | improved 0 | regressed 0 | unchanged 304]
    change:   [min -440.00K | med +954.31K | max +79.11M]
    change %: [min -0.18% | med +0.11% | max +1.81%]

The implementation is split into two parts (refactoring and actual feature) to isolate and assess performance impact in this sensitive code path.

@maksymar maksymar changed the title Maksym/prep chore: prepare for lazy key loading feature May 21, 2025
@maksymar maksymar requested a review from Copilot May 21, 2025 13:33

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2025

canbench 🏋 (dir: .) 04df987 2025-05-21 15:07:53 UTC

./canbench_results.yml is up to date
📦 canbench_results_benchmark.csv available in artifacts

---------------------------------------------------

Summary:
  instructions:
    status:   No significant changes detected 👍
    counts:   [total 304 | new 0 | improved 0 | regressed 0 | unchanged 304]
    change:   [min -440.00K | med +954.31K | max +79.11M]
    change %: [min -0.18% | med +0.11% | max +1.81%]

  heap_increase:
    status:   No significant changes detected 👍
    counts:   [total 304 | new 0 | improved 0 | regressed 0 | unchanged 304]
    change:   [min 0 | med 0 | max 0]
    change %: [min 0.00% | med 0.00% | max 0.00%]

  stable_memory_increase:
    status:   No significant changes detected 👍
    counts:   [total 304 | new 0 | improved 0 | regressed 0 | unchanged 304]
    change:   [min 0 | med 0 | max 0]
    change %: [min 0.00% | med 0.00% | max 0.00%]

---------------------------------------------------
CSV results saved to canbench_results.csv

@maksymar maksymar changed the title chore: prepare for lazy key loading feature chore: prepare for adding key lazy-loading May 21, 2025
@maksymar maksymar changed the title chore: prepare for adding key lazy-loading refactor: prepare for adding key lazy-loading May 21, 2025
@maksymar maksymar changed the title refactor: prepare for adding key lazy-loading refactor: prepare for lazy key loading May 21, 2025
@maksymar maksymar requested a review from Copilot May 21, 2025 15:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the B‑Tree node implementation to prepare for lazy key loading by introducing a new LazyObject structure and unifying the API for handling keys and values.

  • Introduces LazyObject and LazyValue, replacing earlier Value handling for lazy loading.
  • Unifies API methods like get_key/get_value, key/value, and readies upcoming extract_key/extract_value.
  • Updates tests and iterators to account for the new API, including changes to expected types.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

File Description
src/btreemap/node/v2.rs Replaces Value with LazyValue in entry loading and writes for lazy operations.
src/btreemap/node/v1.rs Updates lazy loading from Value to LazyValue and uses get_key for key ordering.
src/btreemap/node.rs Refactors APIs to use LazyObject/LazyValue; adjusts keys(), search(), and entry methods.
src/btreemap.rs Adjusts test assertions to expect references for keys after API changes.
Comments suppressed due to low confidence (1)

src/btreemap.rs:3274

  • [nitpick] The test has been updated to use a slice reference for keys. Confirm that this change aligns with the new API design and that all parts of the codebase expecting key ownership have been appropriately refactored.
assert_eq!(root.keys(), vec![&[6; 10_000]]);

Comment thread src/btreemap/node.rs
Comment thread src/btreemap/node.rs
@maksymar maksymar marked this pull request as ready for review May 21, 2025 15:17
@maksymar maksymar requested a review from a team as a code owner May 21, 2025 15:17
@maksymar maksymar requested a review from dsarlis May 21, 2025 15:23
Copy link
Copy Markdown
Contributor

@berestovskyy berestovskyy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@maksymar maksymar merged commit 737d8ac into main May 21, 2025
12 checks passed
@maksymar maksymar deleted the maksym/prep branch May 21, 2025 15:37
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