-
Notifications
You must be signed in to change notification settings - Fork 666
fix(db): reuse ReadOptions in ColumnDbSnapshot to reduce GC pressure #10894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
97fe652
d53729b
99f0cb3
570a9bb
4c697c3
9518077
60acfff
6ded7eb
863e66d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |||||||||||||||||||
| using System; | ||||||||||||||||||||
| using System.Collections.Generic; | ||||||||||||||||||||
| using System.Linq; | ||||||||||||||||||||
| using System.Threading; | ||||||||||||||||||||
| using FastEnumUtility; | ||||||||||||||||||||
| using Nethermind.Core; | ||||||||||||||||||||
| using Nethermind.Db.Rocks.Config; | ||||||||||||||||||||
|
|
@@ -17,6 +18,11 @@ public class ColumnsDb<T> : DbOnTheRocks, IColumnsDb<T> where T : struct, Enum | |||||||||||||||||||
| { | ||||||||||||||||||||
| private readonly IDictionary<T, ColumnDb> _columnDbs = new Dictionary<T, ColumnDb>(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Cached for ColumnDbSnapshot to avoid per-snapshot recomputation. | ||||||||||||||||||||
| // Initialized once on first CreateSnapshot call; both fields are idempotent (same result from any thread). | ||||||||||||||||||||
| private volatile T[]? _cachedColumnKeys; | ||||||||||||||||||||
| private volatile int _cachedMaxOrdinal = -1; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| public ColumnsDb(string basePath, DbSettings settings, IDbConfig dbConfig, IRocksDbConfigFactory rocksDbConfigFactory, ILogManager logManager, IReadOnlyList<T> keys, IntPtr? sharedCache = null) | ||||||||||||||||||||
| : this(basePath, settings, dbConfig, rocksDbConfigFactory, logManager, ResolveKeys(keys), sharedCache) | ||||||||||||||||||||
| { | ||||||||||||||||||||
|
|
@@ -158,31 +164,121 @@ IColumnDbSnapshot<T> IColumnsDb<T>.CreateSnapshot() | |||||||||||||||||||
| return new ColumnDbSnapshot(this, snapshot); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private class ColumnDbSnapshot( | ||||||||||||||||||||
| ColumnsDb<T> columnsDb, | ||||||||||||||||||||
| Snapshot snapshot | ||||||||||||||||||||
| ) : IColumnDbSnapshot<T> | ||||||||||||||||||||
| private class ColumnDbSnapshot : IColumnDbSnapshot<T> | ||||||||||||||||||||
| { | ||||||||||||||||||||
| private readonly Dictionary<T, IReadOnlyKeyValueStore> _columnDbs = columnsDb.ColumnKeys.ToDictionary(k => k, k => | ||||||||||||||||||||
| (IReadOnlyKeyValueStore)new RocksDbReader( | ||||||||||||||||||||
| columnsDb, | ||||||||||||||||||||
| () => | ||||||||||||||||||||
| private readonly Snapshot _snapshot; | ||||||||||||||||||||
| private readonly ReadOptions _sharedReadOptions; | ||||||||||||||||||||
| private readonly ReadOptions _sharedCacheMissReadOptions; | ||||||||||||||||||||
| private int _disposed; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Use a flat array indexed by enum ordinal instead of Dictionary<T, IReadOnlyKeyValueStore>. | ||||||||||||||||||||
| // This eliminates the dictionary + backing array allocation per snapshot. | ||||||||||||||||||||
| private readonly RocksDbReader[] _readers; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| public ColumnDbSnapshot(ColumnsDb<T> columnsDb, Snapshot snapshot) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| _snapshot = snapshot; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Create two shared ReadOptions for all column readers instead of 2 per reader. | ||||||||||||||||||||
| // ReadOptions in RocksDbSharp has a finalizer but no IDisposable — creating many | ||||||||||||||||||||
| // short-lived instances causes Gen1/Gen2 GC pressure from finalizer queue buildup. | ||||||||||||||||||||
| _sharedReadOptions = CreateReadOptions(columnsDb, snapshot); | ||||||||||||||||||||
| _sharedCacheMissReadOptions = CreateReadOptions(columnsDb, snapshot); | ||||||||||||||||||||
| _sharedCacheMissReadOptions.SetFillCache(false); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Single shared delegate for GetViewBetween — avoids per-reader closure allocation. | ||||||||||||||||||||
| // Note: each GetViewBetween call still creates a new ReadOptions with a finalizer; | ||||||||||||||||||||
| // that is pre-existing behavior not addressed by this PR. | ||||||||||||||||||||
| Func<ReadOptions> readOptionsFactory = () => CreateReadOptions(columnsDb, snapshot); | ||||||||||||||||||||
| T[] keys = CreateKeyCache(columnsDb); | ||||||||||||||||||||
| columnsDb._cachedMaxOrdinal = GetCachedMaxOrdinal(columnsDb, keys); | ||||||||||||||||||||
| _readers = CreateReaders(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| static ReadOptions CreateReadOptions(ColumnsDb<T> columnsDb, Snapshot snapshot) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| ReadOptions options = new ReadOptions(); | ||||||||||||||||||||
| options.SetVerifyChecksums(columnsDb.VerifyChecksum); | ||||||||||||||||||||
| options.SetSnapshot(snapshot); | ||||||||||||||||||||
| return options; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Cache column keys and max ordinal on the parent ColumnsDb to avoid per-snapshot | ||||||||||||||||||||
| // recomputation. The race is benign (both threads compute identical results) and | ||||||||||||||||||||
| // volatile ensures visibility across cores. | ||||||||||||||||||||
| static T[] CreateKeyCache(ColumnsDb<T> columnsDb) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| T[]? keys = columnsDb._cachedColumnKeys; | ||||||||||||||||||||
| if (keys is null) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| ReadOptions options = new ReadOptions(); | ||||||||||||||||||||
| options.SetVerifyChecksums(columnsDb.VerifyChecksum); | ||||||||||||||||||||
| options.SetSnapshot(snapshot); | ||||||||||||||||||||
| return options; | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| columnFamily: columnsDb._columnDbs[k]._columnFamily)); | ||||||||||||||||||||
| IDictionary<T, ColumnDb> columnDbs = columnsDb._columnDbs; | ||||||||||||||||||||
| keys = new T[columnDbs.Count]; | ||||||||||||||||||||
| int idx = 0; | ||||||||||||||||||||
| foreach (T key in columnDbs.Keys) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| keys[idx++] = key; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| columnsDb._cachedColumnKeys = keys; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return keys; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| static int GetCachedMaxOrdinal(ColumnsDb<T> columnsDb, T[] keys) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if (columnsDb._cachedMaxOrdinal >= 0) return columnsDb._cachedMaxOrdinal; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| int max = 0; | ||||||||||||||||||||
| for (int i = 0; i < keys.Length; i++) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| max = Math.Max(max, EnumToInt(keys[i])); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return max; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Build flat array of readers indexed by column ordinal | ||||||||||||||||||||
| RocksDbReader[] CreateReaders() | ||||||||||||||||||||
| { | ||||||||||||||||||||
| RocksDbReader[] readers = new RocksDbReader[columnsDb._cachedMaxOrdinal + 1]; | ||||||||||||||||||||
| for (int i = 0; i < keys.Length; i++) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| T k = keys[i]; | ||||||||||||||||||||
| readers[EnumToInt(k)] = new RocksDbReader( | ||||||||||||||||||||
| columnsDb, | ||||||||||||||||||||
| _sharedReadOptions, | ||||||||||||||||||||
| _sharedCacheMissReadOptions, | ||||||||||||||||||||
| readOptionsFactory, | ||||||||||||||||||||
| columnFamily: columnsDb._columnDbs[k]._columnFamily); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return readers; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| public IReadOnlyKeyValueStore GetColumn(T key) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| return _columnDbs[key]; | ||||||||||||||||||||
| ObjectDisposedException.ThrowIf(_disposed != 0, this); | ||||||||||||||||||||
| return _readers[EnumToInt(key)]; | ||||||||||||||||||||
|
||||||||||||||||||||
| return _readers[EnumToInt(key)]; | |
| int ordinal = EnumToInt(key); | |
| if ((uint)ordinal >= (uint)_readers.Length || _readers[ordinal] is null) | |
| { | |
| throw new KeyNotFoundException($"Column '{key}' is not configured."); | |
| } | |
| return _readers[ordinal]; |
Outdated
Copilot
AI
Mar 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DestroyReadOptions calls Native.Instance.rocksdb_readoptions_destroy(...) directly, bypassing the _rocksDbNative instance used throughout DbOnTheRocks/ColumnsDb for native interop (and which can be injected). For consistency and to preserve the ability to use a non-default Native implementation, consider using the owning DB's _rocksDbNative (e.g., capture it in ColumnDbSnapshot and use that here).
Outdated
Copilot
AI
Mar 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnumToInt uses Convert.ToInt32(value) on a generic enum T. This binds to the Convert.ToInt32(object) overload, which boxes the enum value and can introduce per-call allocations in a hot path (constructor loop + every GetColumn). Since the goal of this PR is to reduce GC pressure, consider switching to a non-boxing conversion (e.g., a cached converter per T based on the enum underlying type, or constrain/assume int underlying for column enums and use a direct cast/Unsafe conversion).
| private static int EnumToInt(T value) => Convert.ToInt32(value); | |
| private static int EnumToInt(T value) => value.ToInt32(); |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,22 +22,36 @@ public class RocksDbReader : ISortedKeyValueStore | |||||||
| private readonly DbOnTheRocks.IteratorManager? _iteratorManager; | ||||||||
| private readonly ColumnFamilyHandle? _columnFamily; | ||||||||
|
|
||||||||
| readonly ReadOptions _options; | ||||||||
| readonly ReadOptions _hintCacheMissOptions; | ||||||||
| private readonly ReadOptions _options; | ||||||||
| private readonly ReadOptions _hintCacheMissOptions; | ||||||||
|
|
||||||||
| public RocksDbReader(DbOnTheRocks mainDb, | ||||||||
| Func<ReadOptions> readOptionsFactory, | ||||||||
| DbOnTheRocks.IteratorManager? iteratorManager = null, | ||||||||
| ColumnFamilyHandle? columnFamily = null) | ||||||||
| : this(mainDb, readOptionsFactory(), readOptionsFactory(), readOptionsFactory, iteratorManager, columnFamily) | ||||||||
| { | ||||||||
| _hintCacheMissOptions.SetFillCache(false); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Constructor that accepts pre-created <see cref="ReadOptions"/> instead of a factory. | ||||||||
| /// Used by <see cref="ColumnsDb{T}.ColumnDbSnapshot"/> to share a single pair of ReadOptions | ||||||||
| /// across all column readers, avoiding per-reader native handle allocation and finalizer pressure. | ||||||||
| /// </summary> | ||||||||
| public RocksDbReader(DbOnTheRocks mainDb, | ||||||||
| ReadOptions options, | ||||||||
| ReadOptions hintCacheMissOptions, | ||||||||
| Func<ReadOptions> readOptionsFactory, | ||||||||
| DbOnTheRocks.IteratorManager? iteratorManager = null, | ||||||||
| ColumnFamilyHandle? columnFamily = null) | ||||||||
LukaszRozmej marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| { | ||||||||
| _mainDb = mainDb; | ||||||||
| _readOptionsFactory = readOptionsFactory; | ||||||||
| _iteratorManager = iteratorManager; | ||||||||
| _columnFamily = columnFamily; | ||||||||
|
|
||||||||
| _options = readOptionsFactory(); | ||||||||
| _hintCacheMissOptions = readOptionsFactory(); | ||||||||
| _hintCacheMissOptions.SetFillCache(false); | ||||||||
| _options = options; | ||||||||
| _hintCacheMissOptions = hintCacheMissOptions; | ||||||||
|
||||||||
| _hintCacheMissOptions = hintCacheMissOptions; | |
| _hintCacheMissOptions = hintCacheMissOptions; | |
| _hintCacheMissOptions.SetFillCache(false); |
Uh oh!
There was an error while loading. Please reload this page.