Repository navigation
Refactor write memory accounting into named memory blocks - #18848
hongzhi-gao wants to merge 4 commits into
Conversation
Caideyipi
left a comment
There was a problem hiding this comment.
Reviewed commit ef0d5bfa35c3 against the PR's merge base eb0e3f1f2080. I did not find an actionable regression introduced by this change.
The review covered row, mixed-row and split-tablet memory admission/WAL rollback; MemTable and metadata release; WAL snapshot format compatibility; idle-array accounting; and flush-worker failure/interruption cleanup.
Validation
89 focused tests passed:
- 59 DataNode tests across
WriteMemoryBlockTest,MemTableMemoryBlockTest,WriteMemoryLifecycleTest,PrimitiveArrayManagerTest,MemTableFlushTaskTest, andTsFileProcessorTest. - 3 tests in
ReadOnlyMemoryBlockTest. - 27 tests in
AlignedTVListTest.
git diff --check also passed.
Full-reactor test compilation could not complete because the local JVM failed to reserve native memory. The integration tests in this PR were not rerun as part of this review.
| private boolean awaitExit() { | ||
| boolean interrupted = false; | ||
| while (true) { | ||
| try { | ||
| finished.get(); | ||
| return interrupted; | ||
| } catch (InterruptedException e) { | ||
| interrupted = true; | ||
| } catch (ExecutionException e) { | ||
| throw new AssertionError(e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why does the interruption not exit?
And why is the ExecutionException wrapped with AssertionError?
There was a problem hiding this comment.
Replaced the exit wait with a CountDownLatch, so worker failures are propagated through subTaskFailure to syncFlushMemTable rather than being represented as an AssertionError in cleanup. The caller still has to wait after interruption: cancellation only requests worker interruption, and the worker may still be using the writer/buffers. Returning here would release its flush-memory reservation before those resources stop being used. Cleanup waits for both workers to exit, then restores the caller's interrupt flag and propagates the original failure/interruption. The regression now also interrupts the caller again during cancellation and checks that the reservation remains live until the I/O worker exits.
Implemented in f1945e5201.
Validation: 87 focused tests passed; full-reactor test compilation passed for both the default and Chinese locales; all 4 IoTDBWriteMemoryLifecycleIT cases passed in 1 ConfigNode + 1 DataNode, including forced-restart WAL recovery. Independent Codex review found no actionable issues.
|
|
||
| /** Adds array/TVList structural memory; Binary payload has a separate block. */ | ||
| void addTVListRamCost(long cost); |
There was a problem hiding this comment.
Add a structureMemoryBlock?
There was a problem hiding this comment.
Added getStructureMemoryBlock() and the MEMTABLE_STRUCTURE category. Aligned TVList container estimates are now accounted separately from arrays/bitmaps and Binary payloads, using the existing before/after reconciliation for working and sorted TVLists. This keeps the aggregate estimate unchanged rather than adding a second charge. The block participates in attach, clear and release; WAL snapshots retain the existing aggregate field and recover it into the recovered-total block. Extended coverage checks category totals, WAL-failure rollback, clear/release, and both old/current snapshot readers.
Implemented in f1945e5201.
| private static long arraySize(Object array) { | ||
| // Use typed array lengths on this hot path instead of reflective shallowSizeOf(Object). | ||
| if (array instanceof long[]) { | ||
| return RamUsageEstimator.sizeOf((long[]) array); | ||
| } else if (array instanceof int[]) { | ||
| return RamUsageEstimator.sizeOf((int[]) array); | ||
| } else if (array instanceof boolean[]) { | ||
| return RamUsageEstimator.sizeOf((boolean[]) array); | ||
| } else if (array instanceof float[]) { | ||
| return RamUsageEstimator.sizeOf((float[]) array); | ||
| } else if (array instanceof double[]) { | ||
| return RamUsageEstimator.sizeOf((double[]) array); | ||
| } | ||
| // Binary[] entries have been cleared; only the shallow reference array belongs to the pool. | ||
| return RamUsageEstimator.shallowSizeOf((Binary[]) array); | ||
| } |
There was a problem hiding this comment.
How about caching the size calculators in an array consistent with POOLED_ARRAYS to reduce unnecessary type judgement.
There was a problem hiding this comment.
Added cached typed size calculators indexed by TSDataType.serialize(), matching POOLED_ARRAYS. Borrow/return now use the calculator for the selected pool instead of repeating the instanceof chain to calculate its size. The calculators still use the actual array length, and Binary[] accounting remains shallow after clearing its elements. Added coverage for the supported types and their primitive/Binary pool aliases; existing variable-length and concurrent pool-accounting regressions also pass.
Implemented in f1945e5201.
Preserve explicit data-model handling and flush worker lifetime accounting. Adapt PR-only tests to the updated flush and memchunk APIs.
Description
Write-memory accounting is spread across MemTables, TsFileProcessors and DataRegions, requiring callers to update several counters together. This change introduces named, categorized owner blocks that propagate usage to their accounting scope and tie rollback/release to the owner's lifecycle.
Accounting and lifecycle
WriteMemoryBlockandWriteMemoryManagerfor MemTable arrays, Binary values, recovered totals, TsFile metadata and flush temporary reservations. Pending reservations count toward admission but become used MemTable memory only after admission succeeds. WAL failures roll back the admitted deltas.Array-pool accounting and hot paths
Count idle arrays by actual shallow size under the existing per-type queue locks. A new
ReadOnlyMemoryBlockexposes an exact snapshot to the existing memory-manager tree; readers acquire queue locks in a fixed order. This avoids an additional global atomic update on each array borrow/return.WriteMemoryBlockuses a monitor-guardedlong; region totals remain atomic. Zero-delta writes skip reservation creation while retaining pressure checks.SQL, configured memory limits, file/WAL formats and the existing admission policy are unchanged. Accounting follows write ownership, not total JVM heap liveness. New exception messages are provided in both English and Chinese.
Verification
Added 4 lifecycle integration tests, all passed in a real 1 ConfigNode + 1 DataNode environment using the repository’s fixtures, run through JUnitCore.
The tests cover:
Assertions check both query results and live database write-memory metrics, including memory charged by writes and released after flush or rejection.
Performance observations
Compared with
eb0e3f1f2080e34bf5593bfd1cf63b62ecdba2e0, using the same workload/configuration and three interleaved runs per version:Environment: shared Xeon Gold 6130 KVM host, 32 vCPUs, JDK 21, 1C1D/SimpleConsensus, ASYNC WAL, memory control enabled, 2 GiB DataNode heap, array size 64, MemTable threshold 32 MiB, compaction and timed flush disabled. Fixed per-writer warmup/measurement calls were 50k/100k for non-aligned Tablet, 125k/250k for aligned rows, and 20k/40k for aligned Tablet. Each run checked acknowledged row counts through queries after a final FLUSH.
These are controlled class-overlay comparisons, not two independent clean distribution builds. The throughput window excludes the final FLUSH; it does not measure synchronous durability throughput. Shared-host variability and existing QueryId waits affect single-row results. The repeated throughput regression observed during development did not recur in the final comparisons, but the latency increases above remain review considerations; these measurements do not establish a universal speedup.
This PR has:
Key changed/added classes
WriteMemoryBlock,WriteMemoryManager,AbstractMemTable,TsFileProcessor,DataRegionInfo,TsFileProcessorInfo,MemTableFlushTask,SystemInfo,MemoryControlledWALEntryQueue,PrimitiveArrayManager,ReadOnlyMemoryBlock,MemoryManager.