Range index integration: design doc + prototype#1613
Draft
Conversation
Contributor
kevin-montrose
left a comment
kevin-montrose
left a comment
There was a problem hiding this comment.
Generally looks good to me - couple questions to poke at, but no blockers that I see.
badrishc
force-pushed
the
badrishc/bf-tree-integration-plan
branch
from
320af88 to
90fbe55
Compare
Resolves conflicts in IGarnetApi.cs, FunctionsState.cs, RMWMethods.cs, and VarLenInputMethods.cs — keeps both RangeIndex and VectorSet additions. Removes duplicate ReadOptimizedLock.cs (dev canonical version in Synchronization/ subdirectory). Updates global.json to match installed SDK. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
VectorSet implementation has been merged to dev. Update all vectorApiPoC-storeV2 references to dev, remove 'prototype' language, update ReadOptimizedLock note (now in libs/common/Synchronization/). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move RangeIndexManager creation to GarnetServer.CreateDatabase (before store) and pass it through GarnetRecordDisposer and GarnetDatabase, so the disposer's struct copies in readonly fields all share the same manager reference. GarnetRecordDisposer: - Holds RangeIndexManager reference (survives struct copy into readonly fields) - DisposeOnPageEviction=true when manager is set - DisposeRecord frees native BfTree memory on page eviction via DisposeTreeIfOwned, which unregisters and disposes the tree if the handle is valid and belongs to this process instance RangeIndexManager.Index: - ClearTreeHandle: zeros handle in a stub span - DisposeTreeIfOwned: unregisters and disposes BfTree if owned PostCopyUpdater clears old record's TreeHandle after CAS succeeds, so eviction of the old page sees handle=0 and skips disposal. This avoids double-free (new record owns the tree) and is safe against CAS failure (handle is only cleared after CAS succeeds). Tests: 4 new eviction tests verifying BfTree eviction doesn't crash, delete+eviction works, multiple indexes freed cleanly, and create/delete/recreate cycles under memory pressure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
force-pushed
the
badrishc/bf-tree-integration-plan
branch
from
f9c6697 to
62c9a13
Compare
- RISetInvalidKVFieldTooLongTest: field exceeding MAXKEYLEN returns error - RISetInvalidKVValueTooLongTest: value exceeding MAXRECORD returns error - RISetInvalidKVRecordTooSmallTest: record below MINRECORD returns error - RIConcurrentMultiClientTest: 8 clients x 50 ops across write/read/mixed phases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NeedCopyUpdate returns false for RICREATE, so CopyUpdater and PostCopyUpdater are never reached. Replace the stub-copy logic with a throw and remove the dead PostCopyUpdater ClearTreeHandle call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cherry-pick delete-dispose-infra (storeFunctions.DisposeRecord in InternalDelete) and apply BfTree-specific changes: - Remove manual BfTree cleanup from MainStore and UnifiedStore InPlaceDeleters — now handled by GarnetRecordDisposer.DisposeRecord which is called by Tsavorite for both mutable and immutable deletes - Expand GarnetRecordDisposer to accept both RangeIndexManager and CacheSizeTrackerHolder - Add tests verifying DEL on BfTree keys in both mutable and read-only regions frees native resources immediately Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ests" This reverts commit 2703396.
Centralize record disposal at delete/expiration sites: - hlog.DisposeRecord(Deleted) now calls storeFunctions.DisposeRecord before ClearHeapFields, giving the application a single callback for cache-size tracking, BfTree cleanup, etc. - InternalDelete: single DisposeRecord(Deleted) immediately after InPlaceDeleter (mutable) and before Seal (tombstone). Remove the separate else-branch DisposeRecord. - HandleRecordElision: remove all DisposeRecord calls — record is already cleaned at the delete site before elision/freelist logic. - TryTransferToFreeList: remove DisposeRecord(RevivFreeList) — record already cleaned. - InternalRMW: add DisposeRecord(Deleted) for ExpireAndStop and ExpireAndResume at their respective sites. Replace direct ClearValueIfHeap(CopyUpdated) calls with DisposeRecord(CopyUpdated). - Remove all manual heap disposal (AddHeapSize, DisposeValueObject, ClearValueIfHeap) from Garnet InPlaceDeleter and RMW expiration paths in ObjectStore and UnifiedStore session functions. - GarnetRecordDisposer.DisposeRecord handles heap-size tracking only for DisposeReason.Deleted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cherry-pick delete-dispose-infra and apply BfTree-specific changes: - Remove manual BfTree cleanup from MainStore and UnifiedStore InPlaceDeleters — now handled by GarnetRecordDisposer.DisposeRecord which is called by Tsavorite's DisposeRecord(Deleted) at the delete site - Expand GarnetRecordDisposer to accept both RangeIndexManager and CacheSizeTrackerHolder - Add tests verifying DEL on BfTree keys in both mutable and read-only regions frees native resources immediately Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
force-pushed
the
badrishc/bf-tree-integration-plan
branch
from
19524d3 to
8ab0283
Compare
badrishc
force-pushed
the
badrishc/bf-tree-integration-plan
branch
from
e8a2e1f to
e1baf64
Compare
Centralize record disposal at delete/expiration sites: - hlog.DisposeRecord(Deleted) now calls storeFunctions.DisposeRecord before ClearHeapFields, giving the application a single callback for cache-size tracking, BfTree cleanup, etc. - InternalDelete: single DisposeRecord(Deleted) immediately after InPlaceDeleter (mutable) and before Seal (tombstone). Remove the separate else-branch DisposeRecord. - HandleRecordElision: remove all DisposeRecord calls — record is already cleaned at the delete site before elision/freelist logic. - TryTransferToFreeList: remove DisposeRecord(RevivFreeList) — record already cleaned. - InternalRMW: add DisposeRecord(Deleted) for ExpireAndStop and ExpireAndResume at their respective sites. Replace direct ClearValueIfHeap(CopyUpdated) calls with DisposeRecord(CopyUpdated). - Remove all manual heap disposal (AddHeapSize, DisposeValueObject, ClearValueIfHeap) from Garnet InPlaceDeleter and RMW expiration paths in ObjectStore and UnifiedStore session functions. - GarnetRecordDisposer.DisposeRecord handles heap-size tracking only for DisposeReason.Deleted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
force-pushed
the
badrishc/bf-tree-integration-plan
branch
from
e1baf64 to
5694911
Compare
DisposeRecord(CopyUpdated) calls ClearHeapFields with clearKey=true (since reason != Deleted), which clears overflow keys on source records that may still be in-chain. Revert to the original ClearValueIfHeap pattern which only clears the value, preserving keys for traceback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- ObjectAllocatorImpl.DisposeRecordsInRangeForEviction: skip tombstoned records — they were already disposed with DisposeReason.Deleted at the delete site. - GarnetRecordDisposer: BfTree disposal explicitly gated on Deleted and PageEviction reasons. Non-deleted evicted BfTree records are freed on eviction; deleted ones were already freed at delete time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
badrishc
force-pushed
the
badrishc/bf-tree-integration-plan
branch
from
13d1e89 to
40df5a4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.