Skip to content

Fix snapshot point read cache options - #1079

Open
Jim8y wants to merge 3 commits into
master-n3from
fix-snapshot-read-cache
Open

Fix snapshot point read cache options#1079
Jim8y wants to merge 3 commits into
master-n3from
fix-snapshot-read-cache

Conversation

@Jim8y

@Jim8y Jim8y commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Keep fill-cache disabled for snapshot scans only.
  • Use separate snapshot read options with default cache behavior for point reads in LevelDBStore and RocksDBStore.
  • Add storage tests covering the scan and point-read option separation.

Testing

  • dotnet test tests/Neo.Plugins.Storage.Tests/Neo.Plugins.Storage.Tests.csproj

@github-actions github-actions Bot added the N3 label Jun 20, 2026
@codecov

codecov Bot commented Jun 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 50.42%. Comparing base (be365a2) to head (3a8cbff).

Files with missing lines Patch % Lines
plugins/LevelDBStore/Plugins/Storage/Snapshot.cs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           master-n3    #1079      +/-   ##
=============================================
+ Coverage      50.34%   50.42%   +0.08%     
=============================================
  Files            280      280              
  Lines          16473    16477       +4     
  Branches        2132     2132              
=============================================
+ Hits            8293     8309      +16     
+ Misses          7619     7605      -14     
- Partials         561      563       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@erikzhang

Copy link
Copy Markdown
Member

What improvements have been made?

@Jim8y

Jim8y commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

The improvement is in the snapshot point-read path.

Before this change, each storage snapshot used a single read option with FillCache=false. That option is appropriate for scan/iterator reads, but it was also used by Contains and TryGet. As a result, repeated point reads through a snapshot bypassed the LevelDB/RocksDB block cache.

The concrete symptom this can cause is high disk read I/O during transaction-heavy consensus processing. The DBFT path does many snapshot point reads while checking existing transactions, conflict hashes, and transaction verification state. When those reads do not populate the DB cache, the same hot storage blocks can be read/decompressed repeatedly instead of being served from the DB cache. On busy nodes this may show up as saturated read I/O and delayed or unresponsive consensus handling.

This PR keeps FillCache=false for scan paths only:

  • Find / iterator reads still use no-fill-cache read options to avoid cache pollution during scans.
  • Contains / TryGet now use separate snapshot read options with the default fill-cache behavior.
  • The same separation is applied to both LevelDBStore and RocksDBStore.

I also ran a local microbenchmark for repeated snapshot point reads. This is not a full chain benchmark, but it matches the hot point-read pattern this PR changes.

Environment:

  • macOS arm64
  • .NET SDK 10.0.108
  • Release build
  • 100,000 entries, 512 hot keys
  • 10 warmup repetitions, then 7 measured rounds
  • Each round: 512,000 point operations (TryGet + Contains)
  • Result below uses median ops/sec
Store Before After Change
LevelDBStore 1,345,771 ops/s 1,864,834 ops/s +38.6%
RocksDBStore 343,553 ops/s 1,353,306 ops/s +293.9%

The main goal is not to speed up scans. The goal is to avoid making normal snapshot point reads behave like bulk scans from the cache perspective.

@shargon

shargon commented Jun 22, 2026

Copy link
Copy Markdown
Member

But then Contains is not cached?

public class SnapshotReadOptionsTest
{
[TestMethod]
public void LevelDbSnapshotUsesDedicatedReadOptionsForScanAndPointReads()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[suggestion] LevelDbSnapshotUsesDedicatedReadOptionsForScanAndPointReads and RocksDbSnapshotUsesDedicatedReadOptionsForScanAndPointReads never construct a snapshot and never inspect fill-cache. They only scan method IL for the _scan* vs _point* field tokens. A constructor that set FillCache=false on the point-read options (or forgot SetFillCache(false) on the scan options) would still pass, which is the behavior this PR is meant to lock in. UsesField also matches a metadata token anywhere in the body (no ldfld/ldflda check) and AssertMethodUsesOnly treats “any overload/state-machine method references the expected field” as success, so one TryGet overload could silently miss the point-read options if the other hit.

Suggestion: Also scan the constructors (or otherwise assert setup) so scan options are the only ones that disable fill-cache, and require every TryGet overload to load the point-read field. Prefer opcode-aware ldfld matching over a raw 4-byte token search.

@shargon

shargon commented Aug 27, 2026

Copy link
Copy Markdown
Member

But then Contains is not cached?

ping @Jim8y

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants