From c0dc2afc27bf21e2bd6eb33fc3ba0e8568c3bb6c Mon Sep 17 00:00:00 2001 From: MPCoreDeveloper Date: Fri, 4 Sep 2026 09:46:23 +0200 Subject: [PATCH] perf(index): remove quadratic duplicate-key hash removal on batch DELETE (P5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HashIndex.RemoveBatchKeys/RemoveBatch removed every position from a key's List with one O(list) List.Remove shift per duplicate: O(m·n) for a key holding n rows with m duplicate-key deletions in a single batch. Batch removal now keeps the direct allocation-free path for single-row keys (the common unique-key case, verified benchmark-neutral at ~80K legacy / ~133K fixed-width ops/s on --pk) and defers duplicate-key positions into a per-key set that is applied with one O(list) compaction per key. Regression tests (HashIndexDuplicateKeyBatchDeleteTests) cover full and partial duplicate-group deletes on both index backends (managed List + unsafe native) including a reopen; full suite 1764 tests, 0 failed. --- docs/CHANGELOG.md | 7 + .../EXECUTION_PLAN_UPDATE_DELETE.md | 3 +- src/SharpCoreDB/DataStructures/HashIndex.cs | 126 +++++++++++++----- .../HashIndexDuplicateKeyBatchDeleteTests.cs | 116 ++++++++++++++++ 4 files changed, 215 insertions(+), 37 deletions(-) create mode 100644 tests/SharpCoreDB.Tests/HashIndexDuplicateKeyBatchDeleteTests.cs diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 785ff85b..544137a8 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -27,6 +27,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 runtime `AreRecordsEncrypted` gate (so default-config plaintext databases benefit without `NoEncryptMode`). Fixed-size hash-indexed SET columns are re-pointed with one lock per index (`HashIndex.RemoveBatchKeys`/`AddBatchKeys`). +- **Duplicate-key hash-index removal is no longer quadratic (P5)** - `HashIndex.RemoveBatchKeys`/ + `RemoveBatch` previously removed every position from a key's list with one O(list) shift per + duplicate, i.e. O(m·n) for a key holding n rows with m duplicate-key deletions in one batch. + Batch removal now keeps the direct allocation-free path for single-row keys and defers + duplicate-key positions into a per-key set that is applied in one O(list) compaction. New + regression tests cover full and partial duplicate-group deletes on both index backends + (managed `List` + unsafe native backend) including a reopen; full suite 1764 tests, 0 failed. - **Commit-time tombstones now batch the marker writes (C5)** - the DELETE commit phase read the whole file once (#373) but still applied one 4-byte negative-prefix marker per row (one pwrite each). `TombstoneRecords` now patches every marker into the in-memory snapshot first diff --git a/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md b/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md index b1bd3855..d1837725 100644 --- a/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md +++ b/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md @@ -12,7 +12,8 @@ | #369 | C4 (batch markers + evict-dedup) + B3 (structured delete, geen dubbele parse) | veilig; neutraal binnen ruis op benchmark | | #370 | B1 (key-only decode: alleen PK + hash-indexkolommen) | veilig; neutraal binnen ruis op small-row benchmark | | #376 | **Bulk aflopende PK-delete** (`DeleteRecordsCore` verzamelt PK-sleutels eenmalig; `IIndex.DeleteBulk`/`BTree.DeleteBulk` sorteert aflopend → rechter-bladpad, minder separator-promoties) | correct (identieke keyset, één bezoek per key); fair-PK legacy-DELETE ~69-72K ops/s (binnen ruis op geordende batches) — winst bij ongeordende keysets | -| C5 (open) | **Commit-marker writes gebatcht** (`TombstoneRecords` patcht alle markers eerst in het whole-file snapshot — markers mogen page-grenzen kruisen — en flusht elke geraakte storage-page één keer i.p.v. één 4B-pwrite per marker) | fair-PK (median, zelfde machine): legacy ~70K → **~81K ops/s** (+16%); fixed-width ~97K → **~141K ops/s** (+45%) — DELETE-gap vs SQLite op FW → ~2,4x | +| #377 | **Commit-marker writes gebatcht** (`TombstoneRecords` patcht alle markers eerst in het whole-file snapshot — markers mogen page-grenzen kruisen — en flusht elke geraakte storage-page één keer i.p.v. één 4B-pwrite per marker) | fair-PK (median, zelfde machine): legacy ~70K → **~81K ops/s** (+16%); fixed-width ~97K → **~141K ops/s** (+45%) — DELETE-gap vs SQLite op FW → ~2,4x | +| P5 (deze branch) | **Duplicate-key hash-removal O(m·n) → O(n) per key** (`RemoveBatchKeys`/`RemoveBatch`: directe allocatie-vrije pad voor single-row keys; gedupliceerde keys gedeferred in set + één O(list)-compaction) | correct (regressietests op volle + partiële duplicate-groepen, beide backends, incl. reopen); benchmark-neutraal op unieke keys | **Root cause (niet in Grok-doc):** `ExecuteBatchSQL` draait elke batch in een storage-transactie; zonder #368 deed batch-DELETE nog steeds de #366 full-file compactie (~690 ms in `tableFlushLoop`). Daardoor waren eerdere “winst”-metingen niet-duurzaam/logisch-only. diff --git a/src/SharpCoreDB/DataStructures/HashIndex.cs b/src/SharpCoreDB/DataStructures/HashIndex.cs index 2827bd14..b845c756 100644 --- a/src/SharpCoreDB/DataStructures/HashIndex.cs +++ b/src/SharpCoreDB/DataStructures/HashIndex.cs @@ -240,40 +240,15 @@ public void RemoveBatch(List> rows, long[] positions) { if (rows.Count == 0) return; - _lock.EnterWriteLock(); - try - { - for (int i = 0; i < rows.Count; i++) - { - if (!rows[i].TryGetValue(_columnName, out var key) || key is null) - continue; - - var normalizedKey = NormalizeKey(key); - - if (_useUnsafeEqualityIndex) - { - var keyBytes = BuildUnsafeKey(normalizedKey); - if (_unsafeIndex.Remove(keyBytes, positions[i])) - { - _unsafeTotalRows--; - } - continue; - } - - if (_index.TryGetValue(normalizedKey, out var list)) - { - list.Remove(positions[i]); - if (list.Count == 0) - { - _index.Remove(normalizedKey); - } - } - } - } - finally + // Extract each row's indexed key once (null when the column is absent) and delegate to + // the key-based batch removal, which compacts duplicate-key lists in a single pass. + var keys = new object?[rows.Count]; + for (int i = 0; i < rows.Count; i++) { - _lock.ExitWriteLock(); + rows[i].TryGetValue(_columnName, out keys[i]); } + + RemoveBatchKeys(keys, positions); } /// @@ -292,11 +267,21 @@ internal void RemoveBatchKeys(object?[] keys, long[] positions) _lock.EnterWriteLock(); try { + // Deferred duplicate-key removals. A key whose position list has more than one entry and + // appears more than once in the batch previously cost one O(list) List.Remove shift per + // duplicate — O(m·n) for a key with n rows and m duplicate-key deletions in the batch. + // Instead the first occurrence is removed directly and later occurrences are collected, + // after which the list is compacted in one O(list) pass. Single-row keys (the common + // case for unique-ish indexed values) stay on the allocation-free direct path. + Dictionary>? deferred = null; + for (int i = 0; i < keys.Length; i++) { var key = keys[i]; if (key is null) + { continue; + } var normalizedKey = NormalizeKey(key); @@ -307,15 +292,54 @@ internal void RemoveBatchKeys(object?[] keys, long[] positions) { _unsafeTotalRows--; } + continue; } - if (_index.TryGetValue(normalizedKey, out var list)) + if (!_index.TryGetValue(normalizedKey, out var list)) { - list.Remove(positions[i]); - if (list.Count == 0) + continue; + } + + if (list.Count > 1) + { + deferred ??= new Dictionary>(_comparer); + if (deferred.TryGetValue(normalizedKey, out var dupSet)) + { + (dupSet ??= new HashSet()).Add(positions[i]); + deferred[normalizedKey] = dupSet; + } + else + { + list.Remove(positions[i]); + if (list.Count == 0) + { + _index.Remove(normalizedKey); + } + else + { + deferred.Add(normalizedKey, null); + } + } + + continue; + } + + // Single-row key: direct removal, no duplicate tracking needed. + list.Remove(positions[i]); + if (list.Count == 0) + { + _index.Remove(normalizedKey); + } + } + + if (deferred != null) + { + foreach (var kvp in deferred) + { + if (kvp.Value is not null) { - _index.Remove(normalizedKey); + CompactPositionList(kvp.Key, kvp.Value); } } } @@ -326,6 +350,36 @@ internal void RemoveBatchKeys(object?[] keys, long[] positions) } } + /// + /// Removes every position in from the key's position list with one + /// O(list) compaction pass (value-based membership, so ordering is irrelevant). + /// + private void CompactPositionList(object key, HashSet removed) + { + if (!_index.TryGetValue(key, out var list)) + { + return; + } + + int write = 0; + for (int read = 0; read < list.Count; read++) + { + if (!removed.Contains(list[read])) + { + list[write++] = list[read]; + } + } + + if (write == 0) + { + _index.Remove(key); + } + else if (write < list.Count) + { + list.RemoveRange(write, list.Count - write); + } + } + /// /// Key-based overload of : callers that already know each indexed key /// (e.g. an in-place UPDATE re-point) add all rows with one lock acquisition per index. diff --git a/tests/SharpCoreDB.Tests/HashIndexDuplicateKeyBatchDeleteTests.cs b/tests/SharpCoreDB.Tests/HashIndexDuplicateKeyBatchDeleteTests.cs new file mode 100644 index 00000000..824c2ac6 --- /dev/null +++ b/tests/SharpCoreDB.Tests/HashIndexDuplicateKeyBatchDeleteTests.cs @@ -0,0 +1,116 @@ +// +// Copyright (c) 2026 MPCoreDeveloper. All rights reserved. +// Licensed under the MIT License. See LICENSE file in the project root for full license information. +// +namespace SharpCoreDB.Tests; + +using Microsoft.Extensions.DependencyInjection; +using SharpCoreDB.Interfaces; +using System; +using System.Collections.Generic; +using System.Globalization; +using System.IO; +using Xunit; + +/// +/// Regression coverage for duplicate-key hash-index removal: batch-DELETEs that hit a non-unique +/// indexed column with large position groups exercise HashIndex.RemoveBatchKeys's deferred +/// duplicate-key compaction (previously one O(list) List.Remove shift per duplicate). +/// +public sealed class HashIndexDuplicateKeyBatchDeleteTests : IDisposable +{ + private readonly DatabaseFactory _factory; + private readonly string _dirPath; + + public HashIndexDuplicateKeyBatchDeleteTests() + { + var services = new ServiceCollection(); + services.AddSharpCoreDB(); + _factory = services.BuildServiceProvider().GetRequiredService(); + _dirPath = Path.Combine(Path.GetTempPath(), $"SCDB_HashDup_{Guid.NewGuid():N}"); + } + + public void Dispose() + { + try { if (Directory.Exists(_dirPath)) Directory.Delete(_dirPath, true); } catch { } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void BatchDelete_DuplicatedNameGroups_RemovesOnlyThoseKeys_AcrossReopen(bool useUnsafeEqualityIndex) + { + IDatabase? db = _factory.Create(_dirPath, "pw", isReadOnly: false, + config: new DatabaseConfig + { + NoEncryptMode = true, + AutoFixedWidthRecords = false, + EnableUnsafeEqualityIndex = useUnsafeEqualityIndex, + }); + try + { + db.ExecuteSQL("CREATE TABLE docs (id INTEGER PRIMARY KEY, name TEXT, score REAL)"); + db.ExecuteSQL("CREATE INDEX idx_docs_name ON docs(name)"); + + // 2000 rows, 200 rows per duplicated name group (dup0..dup9). + var stmts = new List(2000); + for (int i = 1; i <= 2000; i++) + { + stmts.Add(string.Format(CultureInfo.InvariantCulture, + "INSERT INTO docs VALUES ({0}, 'dup{1}', {2})", i, i % 10, i * 0.5)); + } + + db.ExecuteBatchSQL(stmts); + db.Flush(); + + Assert.Equal(2000, db.ExecuteQuery("SELECT id FROM docs").Count); + + // Delete three full duplicate groups in one batch: dup1, dup5, dup9 (600 rows). + db.ExecuteBatchSQL( + [ + "DELETE FROM docs WHERE name = 'dup1'", + "DELETE FROM docs WHERE name = 'dup5'", + "DELETE FROM docs WHERE name = 'dup9'", + ]); + db.Flush(); + + Assert.Equal(1400, db.ExecuteQuery("SELECT id FROM docs").Count); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup1'")); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup5'")); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup9'")); + Assert.Equal(200, db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup2'").Count); + Assert.Equal(200, db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup0'").Count); + + // Partial group delete: remove half of dup2 by id, keeping the rest reachable. + var partial = new List(100); + for (int i = 2; i <= 2000; i += 20) + { + partial.Add($"DELETE FROM docs WHERE id = {i}"); + } + + db.ExecuteBatchSQL(partial); + db.Flush(); + Assert.Equal(100, db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup2'").Count); + } + finally { (db as IDisposable)?.Dispose(); } + + // Reopen: tombstoned rows stay gone, live duplicated-key groups stay fully reachable. + db = _factory.Create(_dirPath, "pw", isReadOnly: false, + config: new DatabaseConfig + { + NoEncryptMode = true, + AutoFixedWidthRecords = false, + EnableUnsafeEqualityIndex = useUnsafeEqualityIndex, + }); + try + { + Assert.Equal(1300, db.ExecuteQuery("SELECT id FROM docs").Count); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup1'")); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup5'")); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup9'")); + Assert.Equal(100, db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup2'").Count); + Assert.Equal(200, db.ExecuteQuery("SELECT id FROM docs WHERE name = 'dup0'").Count); + } + finally { (db as IDisposable)?.Dispose(); } + } +}