Cpp code dump - #1
Merged
Merged
Conversation
westonpace
pushed a commit
that referenced
this pull request
Jul 25, 2024
before: <img width="710" alt="Screenshot 2024-07-21 at 4 24 44 PM" src="https://github.com/user-attachments/assets/65a953ac-bbbf-4244-b3fa-d5b7c368806e"> after: <img width="589" alt="Screenshot 2024-07-21 at 4 28 49 PM" src="https://github.com/user-attachments/assets/16a20200-d8f2-4e5a-8e59-2be42281222e"> to reproduce: `cargo run --release --example benchmark ` in `rust/lance-encoding/compression-algo/fsst` machine info: `11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz` `Linux 192 5.10.0-28-amd64 #1 SMP Debian 5.10.209-2 (2024-01-31) x86_64 GNU/Linux`
6 tasks
westonpace
pushed a commit
that referenced
this pull request
May 14, 2026
…es (#6767) ## Summary After a writer flushed a memtable to L0 and an external compactor merged that generation into the base table — legitimately draining `flushed_generations` to empty — a subsequent restart re-replayed the original WAL entries into the new active memtable, duplicating rows on read. Two bugs were interacting: 1. **Disambiguation:** `replay_memtable_from_wal` distinguished "fresh shard" from "flushed and compacted" via `flushed_generations.is_empty()`. That works in a closed-world deployment but breaks the moment an external compactor enters the picture — and the compactor is the *intended* consumer that drains that vector, so the signal is structurally broken under OSS-WAL. 2. **Cursor never advanced:** `MemTableFlusher::flush` read `covered_wal_entry_position` from `memtable.last_flushed_wal_entry_position()`, but that field is only set by the `mark_wal_flushed` test helper. In production it stayed at 0, so `replay_after_wal_entry_position` never advanced past 0. Under 0-based WAL positions this masked bug #1 — both "fresh" and "post-flush-of-0" produced cursor=0. ## Fix - **WAL positions are now 1-based** (`FIRST_WAL_ENTRY_POSITION = 1`). A cursor of `0` unambiguously means "no flush has stamped this shard," so replay collapses to `cursor.saturating_add(1)` without consulting `flushed_generations`. - **`WalFlushHandler::handle`** writes the just-appended position back into `state.last_flushed_wal_entry_position` under the state lock before signalling the completion cell. - **`MemTableFlusher::flush` / `flush_with_indexes`** now take an explicit `covered_wal_entry_position` arg. The production caller derives it per-memtable from the `WalFlushResult` carried in the completion cell — authoritative under concurrent flushes — falling back to `memtable.frozen_at_wal_entry_position()` when freeze did not trigger a flush. - **State seed at open** uses the post-replay WAL tip, not `manifest.wal_entry_position_last_seen` (the latter is bumped on every tailer read and can sit above any flushed generation). - Proto field docs on `ShardManifest.replay_after_wal_entry_position` / `wal_entry_position_last_seen` updated to spell out the 1-based convention and what default-0 means. ## Test plan - [x] Added `test_memtable_replay_skips_entries_after_external_compaction` in `rust/lance/src/dataset/mem_wal/write.rs`: open writer, put rows, close (flush), simulate the compactor by directly committing a manifest with empty `flushed_generations`, reopen, assert the memtable is empty. Fails on the pre-fix code; passes now. - [x] `cargo test -p lance --lib dataset::mem_wal` — 236/236 pass - [x] `cargo test -p lance --lib` — 1600/1600 pass - [x] `cargo test -p lance-index --lib` — 302/302 pass - [x] `cargo clippy --all --tests --benches -- -D warnings` — clean - [x] `cargo fmt --all -- --check` — clean ## Compatibility WAL position numbering changes from 0-based to 1-based. Existing on-disk manifests / WAL files written by the prior `oss-wal-multiplex` code are not migrated — coordinated with downstream consumers (sophon) to start fresh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
geruh
referenced
this pull request
in geruh/lance
May 16, 2026
…es (lance-format#6767) ## Summary After a writer flushed a memtable to L0 and an external compactor merged that generation into the base table — legitimately draining `flushed_generations` to empty — a subsequent restart re-replayed the original WAL entries into the new active memtable, duplicating rows on read. Two bugs were interacting: 1. **Disambiguation:** `replay_memtable_from_wal` distinguished "fresh shard" from "flushed and compacted" via `flushed_generations.is_empty()`. That works in a closed-world deployment but breaks the moment an external compactor enters the picture — and the compactor is the *intended* consumer that drains that vector, so the signal is structurally broken under OSS-WAL. 2. **Cursor never advanced:** `MemTableFlusher::flush` read `covered_wal_entry_position` from `memtable.last_flushed_wal_entry_position()`, but that field is only set by the `mark_wal_flushed` test helper. In production it stayed at 0, so `replay_after_wal_entry_position` never advanced past 0. Under 0-based WAL positions this masked bug #1 — both "fresh" and "post-flush-of-0" produced cursor=0. ## Fix - **WAL positions are now 1-based** (`FIRST_WAL_ENTRY_POSITION = 1`). A cursor of `0` unambiguously means "no flush has stamped this shard," so replay collapses to `cursor.saturating_add(1)` without consulting `flushed_generations`. - **`WalFlushHandler::handle`** writes the just-appended position back into `state.last_flushed_wal_entry_position` under the state lock before signalling the completion cell. - **`MemTableFlusher::flush` / `flush_with_indexes`** now take an explicit `covered_wal_entry_position` arg. The production caller derives it per-memtable from the `WalFlushResult` carried in the completion cell — authoritative under concurrent flushes — falling back to `memtable.frozen_at_wal_entry_position()` when freeze did not trigger a flush. - **State seed at open** uses the post-replay WAL tip, not `manifest.wal_entry_position_last_seen` (the latter is bumped on every tailer read and can sit above any flushed generation). - Proto field docs on `ShardManifest.replay_after_wal_entry_position` / `wal_entry_position_last_seen` updated to spell out the 1-based convention and what default-0 means. ## Test plan - [x] Added `test_memtable_replay_skips_entries_after_external_compaction` in `rust/lance/src/dataset/mem_wal/write.rs`: open writer, put rows, close (flush), simulate the compactor by directly committing a manifest with empty `flushed_generations`, reopen, assert the memtable is empty. Fails on the pre-fix code; passes now. - [x] `cargo test -p lance --lib dataset::mem_wal` — 236/236 pass - [x] `cargo test -p lance --lib` — 1600/1600 pass - [x] `cargo test -p lance-index --lib` — 302/302 pass - [x] `cargo clippy --all --tests --benches -- -D warnings` — clean - [x] `cargo fmt --all -- --check` — clean ## Compatibility WAL position numbering changes from 0-based to 1-based. Existing on-disk manifests / WAL files written by the prior `oss-wal-multiplex` code are not migrated — coordinated with downstream consumers (sophon) to start fresh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 22, 2026
hamersaw
referenced
this pull request
in hamersaw/lance
Jun 4, 2026
…KeyIndex Per jackye1995's review #1 ("composite-key BTreeMemIndex"): instead of a separate parallel skiplist (`PkKeyIndex`), the composite PK index is a plain `BTreeMemIndex` keyed on a synthetic `Binary` column (`__pk_key__`) holding the order-preserving encoded tuple. `pk_key.rs` is now just the encoder (`encode_pk_tuple` + `encode_pk_batch`); the insert path materializes the encoded `Binary` column and feeds the existing index, and the probe seeks with `ScalarValue::Binary(encode_pk_tuple(values))`. Benefits: the composite case reuses `BTreeMemIndex`'s byte backend (incl. the inline-small-key node optimization) and its `to_training_batches`, so the in-memory probe, flush sidecar, and single-column path share one index type and one code path. Arity-split and its single-column memory/typed-fast-path wins are unchanged. Net deletion of the hand-rolled `PkKeyIndex` skiplist. mem_wal suite green; clippy --tests -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 tasks
westonpace
added a commit
that referenced
this pull request
Jul 8, 2026
…rvation (#7675) Raises the per-partition memory pool from 100MB to 150MB and sets the sort spill reservation to 40MB (up from the DataFusion default of 10MB) to give sort operations more headroom while spilling to disk (we should still spill at roughly the same rate). The previous defaults were 100MB / 10MB. This _usually_ worked but certain patterns would lead to false memory exhaustion errors: ``` OSError: LanceError(IO): Resources exhausted: Additional allocation failed for ExternalSorterMerge[0] with top memory consumers (across reservations) as: ExternalSorterMerge[0]#1(can spill: false) consumed 58.5 MB, peak 58.5 MB, ExternalSorter[0]#0(can spill: true) consumed 41.4 MB, peak 89.9 MB. Error: Failed to allocate additional 345.9 KB for ExternalSorterMerge[0] with 27.6 MB already allocated for this reservation - 92.2 KB remain available for the total pool, /home/pace/lance/rust/lance-datafusion/src/chunker.rs:49:46 ``` The problem happens as follows: 1. The sort node accumulates batches of data without modifying them until it determines a spill is needed. During this phase each batch counts double against the pool reservation. This is meant to provide overhead for the later steps. In our above example we can see spilling was triggered at 41.4MB which is about half of the 90MB pool (half, because each batch is counted double) 2. The sort node determines that spilling is needed. First, it must sort the data that has accumulated in memory. Each batch is sorted by itself. This batch sort is in-place and doesn't affect reservations much. 3. A cursor is created for each in-memory batch. The in-memory batches are then fed into a merge sort. 4. The merge sort accumulates batches of data to send to the spill. Once a batch is accumulated it is written to the spill file. Both the cursors and the accumulation require additional space. This is the `ExternalSorterMerge[0]` mentioned above. It is given the overcounting described in step 1. In other words, once this starts, we have half the reservation in `ExternalSorter[0]` and half the reservation in `ExternalSorterMerge[0]`. This "additional space" _should_ be about the same size as the input. This is why we count each batch twice. In practice, the `ExternalSorterMerge[0]` reservation ends up being slightly higher (for various reasons). This is what the `sort_spill_reservation_bytes` is supposed to account for. Datafusion defaults this to 10MB. There is no guidance (and I can't get Claude to come up with any good guidance) as to what this value should be set to. However, 10MB seems like too little. This PR updates it to about 1/3 of the memory pool size. In theory it shouldn't grow proportionally to the memory pool but in practice it seems to. I also really don't want to expose it as yet another knob that users have to tune so I'm hoping 1/3 is slightly conservative but good enough. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
wombatu-kun
referenced
this pull request
in wombatu-kun/lance
Jul 30, 2026
…rvation (lance-format#7675) Raises the per-partition memory pool from 100MB to 150MB and sets the sort spill reservation to 40MB (up from the DataFusion default of 10MB) to give sort operations more headroom while spilling to disk (we should still spill at roughly the same rate). The previous defaults were 100MB / 10MB. This _usually_ worked but certain patterns would lead to false memory exhaustion errors: ``` OSError: LanceError(IO): Resources exhausted: Additional allocation failed for ExternalSorterMerge[0] with top memory consumers (across reservations) as: ExternalSorterMerge[0]#1(can spill: false) consumed 58.5 MB, peak 58.5 MB, ExternalSorter[0]#0(can spill: true) consumed 41.4 MB, peak 89.9 MB. Error: Failed to allocate additional 345.9 KB for ExternalSorterMerge[0] with 27.6 MB already allocated for this reservation - 92.2 KB remain available for the total pool, /home/pace/lance/rust/lance-datafusion/src/chunker.rs:49:46 ``` The problem happens as follows: 1. The sort node accumulates batches of data without modifying them until it determines a spill is needed. During this phase each batch counts double against the pool reservation. This is meant to provide overhead for the later steps. In our above example we can see spilling was triggered at 41.4MB which is about half of the 90MB pool (half, because each batch is counted double) 2. The sort node determines that spilling is needed. First, it must sort the data that has accumulated in memory. Each batch is sorted by itself. This batch sort is in-place and doesn't affect reservations much. 3. A cursor is created for each in-memory batch. The in-memory batches are then fed into a merge sort. 4. The merge sort accumulates batches of data to send to the spill. Once a batch is accumulated it is written to the spill file. Both the cursors and the accumulation require additional space. This is the `ExternalSorterMerge[0]` mentioned above. It is given the overcounting described in step 1. In other words, once this starts, we have half the reservation in `ExternalSorter[0]` and half the reservation in `ExternalSorterMerge[0]`. This "additional space" _should_ be about the same size as the input. This is why we count each batch twice. In practice, the `ExternalSorterMerge[0]` reservation ends up being slightly higher (for various reasons). This is what the `sort_spill_reservation_bytes` is supposed to account for. Datafusion defaults this to 10MB. There is no guidance (and I can't get Claude to come up with any good guidance) as to what this value should be set to. However, 10MB seems like too little. This PR updates it to about 1/3 of the memory pool size. In theory it shouldn't grow proportionally to the memory pool but in practice it seems to. I also really don't want to expose it as yet another knob that users have to tune so I'm hoping 1/3 is slightly conservative but good enough. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> (cherry picked from commit 18381f3)
hamersaw
added a commit
that referenced
this pull request
Aug 26, 2026
…emory (#7831) ## Why MemWAL's backpressure is a concrete struct whose only seam is a per-call closure, and it can see one shard's **row** bytes. That leaves two gaps: 1. **An embedder cannot express its own budget.** A process running many shards has limits lance cannot know — a process-wide memtable total, a page-cache working set. Nothing could represent them, and nothing could refuse a write: the valve only ever blocked, unboundedly. 2. **The bytes it measures are not the bytes that OOM you.** `MemTable::estimated_size` counts buffered batches + the PK bloom filter. Every in-memory index is invisible to it. Gap 2 is not a rounding error. `HnswGraph::try_new` pre-allocates one node per unit of `capacity` (= `max_memtable_rows`), so a vector memtable commits its **entire** graph on the first insert: | `max_memtable_rows` | HNSW on row #1 | |---|---| | 125k | 64.7 MiB | | 500k | 258 MiB | | 1M | 517 MiB | | 2M | 1033 MiB | Measured, not estimated, and identical at dim=768 and dim=8 — vectors are held by reference, so the cost is ~542 B per row of *capacity* regardless of dimension. A memtable holding one row costs the same as a full one, while `estimated_size` reads ~zero. ## What **Accounting** (`resident_bytes` at each layer, all cheap enough for the write path): - **HNSW** — the node arena and lookup slabs are sized from `capacity` at construction, so the total is computed once in the loop `try_new` already runs, plus an atomic for the rebuilt `packed_level0`. - **BTree** — the skiplist arena counts chunks in its cold `grow` path: free per insert, exact for the nodes. - **FTS** — partitions are capped at `MAX_PARTITIONS` and size themselves in O(1); only the mutable tail needed a running counter, kept in step with the existing walk at its single growth point. A test asserts the two agree exactly, including across a freeze. Row bytes keep their data-only meaning — they size the flush unit, so a generation stays a function of the rows in it. They move off `MemTableStats` onto `ShardMemory::row_bytes()`, alongside `index_bytes()`, `frozen_bytes()`, `grace_bytes()` and `retained_bytes()`; `InMemoryMemTableRef::resident_bytes` is the per-memtable total the ceiling is built on. **The seam:** ```rust async fn maybe_apply_backpressure(&self, shard: ShardMemory) -> Result<()> ``` `ShardWriterConfig::backpressure` **replaces** the built-in valve rather than layering on it, so one implementation owns the whole policy. That is why the gate receives a live view of what the calling shard holds: a replacement can enforce a per-shard ceiling in the same place as its process-wide one, without a back-reference into the writer calling it. `ShardMemory` is deliberately live rather than a snapshot — a controller that delays is waiting for those bytes to fall, so a captured copy would never observe the drain. The gate is deliberately **not** told the incoming batch's size. Batches are decoded and resident by the time it runs, so there is nothing left to reserve against — refusing does not un-allocate them, and bounding a single write's memory belongs to the ingress. This is the settled shape rather than a first cut: adding a parameter later would break every external implementor. With nothing injected, `LocalBackpressureController` keeps today's per-shard behaviour, its two write modes covered by a `LocalSource` enum instead of the closure. **Counters.** `unflushed_memtable_bytes` read through a `try_read()` that yields `0` whenever the write lock is held — and tokio's `RwLock` is write-preferring, so also whenever a writer is merely *queued*. It zeroed exactly the shards taking writes, reporting lowest under the heaviest load. Replaced with `active_bytes()` / `frozen_bytes()`: relaxed loads of counters maintained under the write lock, index memory included. **`Error::Backpressure`** (with `is_backpressure()`) makes a rejection distinguishable from a real failure without matching on the message. **Reservations are validated at `open()`.** An HNSW graph is charged from `max_memtable_rows` before its first insert, while only row bytes seal a memtable. A reservation with no room left under `max_unflushed_memtable_bytes` would therefore put a shard over budget at zero rows — nothing to seal, so nothing to flush, so every put stalls and then fails as `Backpressure`, which is supposed to mean "retry later" and never comes true. `open()` now rejects that configuration outright, requiring `index_reserved + max_memtable_size <= max_unflushed_memtable_bytes`, with both figures named in the error. Only the built-in valve reads that ceiling, so the check is skipped when a controller is injected. ## Update: the process-wide counter is gone Earlier revisions carried `ShardWriterConfig::pod_memory_bytes`, an `Arc<AtomicUsize>` every writer added to on insert and subtracted from on flush-commit, so an embedder could read a process-wide total with one load instead of scanning every shard. **It has been removed**, because an incremental counter of that shape has no way back. Nothing releases a writer's residual bytes when it goes away without a final flush — `abort()` is `shutdown_all()` and nothing else, and there is no `Drop` on `MemoryCounters` — so every teardown that skips a flush leaks its memtable into the sum permanently. The downstream embedder evicts that way on four separate paths, and enough of them leave a process refusing every write against a memtable that reads empty. A `Drop` impl would close that particular hole. But the embedder is better served recomputing the total from `memtable_stats()`, which is also what it exports: derived, self-healing within one interval, and identical to its own gauge by construction. So the counter is deleted rather than patched, and the seam is smaller for it — net −41 lines. `ShardMemory` and the per-shard `active`/`frozen` counters are untouched. They are the live per-shard view a controller needs to enforce a per-shard ceiling, and they have no cross-shard lifetime problem. ## Contract changes - *"Never errors due to backpressure"* → an **injected** controller may reject. The built-in valve still never does. - *"Never drops data"* → never drops **acked** data. A rejected write was never accepted. - `active`/`frozen` now count index memory, so the built-in valve is meaningfully tighter on vector tables at a given `max_unflushed_memtable_bytes`. That is the correct direction for a memory valve, but it is a behaviour change worth knowing about. ## Follow-up (not this PR) The graph costs ~5x the adjacency it stores: level-0 links are held three times (`ranked` with distances, `published`, `packed_level0`), and it is ~2-3 allocations per node. The reference implementation for the fix already lives next door in `mem_wal/index/arena_skiplist.rs`, whose BTree gets 4 MiB where HNSW spends 64.7 MiB. Filing separately — this PR only makes the cost *visible*. ## Test `cargo test -p lance --lib -- dataset::mem_wal`. New: HNSW pre-allocation behaviour, FTS counter-vs-walk equality, injected-replaces-default, default-when-none-injected, reject-surfaces-as-Backpressure, and `ShardMemory` liveness across polls (it hangs if the view ever regresses to a copy). `cargo fmt --all` + `cargo clippy -p lance --lib --tests --benches` clean in touched files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Xuanwo <github@xuanwo.io>
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
1 participant
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.
The initial patch to open source C++ codebase.