@@ -16,6 +16,16 @@ private record ReadingChunkInfo(Memory<byte> Chunk)
1616
1717 private readonly Dictionary < ulong , ReadingChunkInfo > _chunkInfoMap = new ( ) ;
1818
19+ // A caller may share one cache across concurrent reads by passing it via
20+ // H5DatasetAccess.ChunkCache, and _chunkInfoMap plus the ConsumedBytes accounting were otherwise
21+ // mutated with no synchronization. The default path never shares a cache - the default factory
22+ // builds one per read - so this only ever bit callers who opted in, and it bit them silently.
23+ //
24+ // A lock is affordable here because of what it guards: a miss costs a chunk read and usually
25+ // decompression, orders of magnitude more than the lock itself. It is deliberately NOT held
26+ // across chunkReader() - see GetChunk.
27+ private readonly object _lock = new ( ) ;
28+
1929 /// <summary>
2030 /// Initializes a new instance of the <see cref="SimpleReadingChunkCache"/> class.
2131 /// </summary>
@@ -41,7 +51,18 @@ public SimpleReadingChunkCache(int chunkSlotCount = 521, ulong byteCount = 1 * 1
4151 /// <summary>
4252 /// Gets the number of chunk slots that have already been consumed.
4353 /// </summary>
44- public int ConsumedSlots => _chunkInfoMap . Count ;
54+ public int ConsumedSlots
55+ {
56+ get
57+ {
58+ // Reading Dictionary.Count while another reader mutates the dictionary is not safe, so
59+ // this observation is synchronized too - it is a diagnostic, never on the read path.
60+ lock ( _lock )
61+ {
62+ return _chunkInfoMap . Count ;
63+ }
64+ }
65+ }
4566
4667 /// <summary>
4768 /// Gets the maximum size of the chunk cache in bytes.
@@ -54,34 +75,70 @@ public SimpleReadingChunkCache(int chunkSlotCount = 521, ulong byteCount = 1 * 1
5475 public ulong ConsumedBytes { get ; private set ; }
5576
5677 /// <inheritdoc />
78+ /// <remarks>
79+ /// Safe to call concurrently. The lock is released around <paramref name="chunkReader" />:
80+ /// holding it across a chunk read (I/O plus decompression) would serialize every reader
81+ /// sharing this cache and so defeat the point of reading in parallel. The cost is that two
82+ /// readers missing on the same chunk at the same time both decode it and one result is
83+ /// discarded - wasted work, never incorrect.
84+ /// <para>
85+ /// Evicting a chunk another reader is still decoding from is likewise safe, but only
86+ /// because cached chunks are plain GC-allocated arrays (see H5D_Chunk.ReadChunk and
87+ /// H5Filter.ExecutePipeline): eviction drops a reference, and the holder's Memory keeps
88+ /// the array alive. Pooling cached chunks would break that, and a lock would then no
89+ /// longer be sufficient.
90+ /// </para>
91+ /// </remarks>
5792 public Memory < byte > GetChunk ( ulong chunkIndex , Func < Memory < byte > > chunkReader )
5893 {
59- if ( _chunkInfoMap . TryGetValue ( chunkIndex , out var chunkInfo ) )
94+ lock ( _lock )
6095 {
61- chunkInfo . LastAccess = Environment . TickCount64 ;
96+ if ( _chunkInfoMap . TryGetValue ( chunkIndex , out var cached ) )
97+ {
98+ cached . LastAccess = Environment . TickCount64 ;
99+
100+ return cached . Chunk ;
101+ }
62102 }
63103
64- else
104+ var buffer = chunkReader ( ) ;
105+
106+ lock ( _lock )
65107 {
66- var buffer = chunkReader ( ) ;
108+ // Another reader may have installed this chunk while we were decoding it. Prefer the
109+ // installed one, so every reader observes the same buffer for a given index.
110+ if ( _chunkInfoMap . TryGetValue ( chunkIndex , out var installed ) )
111+ {
112+ installed . LastAccess = Environment . TickCount64 ;
67113
68- chunkInfo = new ReadingChunkInfo ( buffer ) { LastAccess = Environment . TickCount64 } ;
114+ return installed . Chunk ;
115+ }
69116
117+ var chunkInfo = new ReadingChunkInfo ( buffer ) { LastAccess = Environment . TickCount64 } ;
70118 var chunk = chunkInfo . Chunk ;
71119
72120 if ( ( ulong ) chunk . Length <= ByteCount )
73121 {
74- while ( _chunkInfoMap . Count >= ChunkSlotCount || ByteCount - ConsumedBytes < ( ulong ) chunk . Length )
122+ // Nothing to preempt once the map is empty. Without that guard a cache constructed
123+ // with zero slots - which the constructor allows, and which reads as "do not cache" -
124+ // preempted an empty map and dereferenced the default KeyValuePair.
125+ while ( _chunkInfoMap . Count > 0 &&
126+ ( _chunkInfoMap . Count >= ChunkSlotCount || ByteCount - ConsumedBytes < ( ulong ) chunk . Length ) )
75127 {
76128 Preempt ( ) ;
77129 }
78130
79- ConsumedBytes += ( ulong ) chunk . Length ;
80- _chunkInfoMap [ chunkIndex ] = chunkInfo ;
131+ // Re-checked rather than assumed: with slots available the loop above has already made
132+ // room, but with none it exits on the emptiness guard and this chunk is not cacheable.
133+ if ( _chunkInfoMap . Count < ChunkSlotCount )
134+ {
135+ ConsumedBytes += ( ulong ) chunk . Length ;
136+ _chunkInfoMap [ chunkIndex ] = chunkInfo ;
137+ }
81138 }
82- }
83139
84- return chunkInfo . Chunk ;
140+ return chunk ;
141+ }
85142 }
86143
87144 private void Preempt ( )
0 commit comments