wernerdv commented on code in PR #13554:
URL: https://github.com/apache/ignite/pull/13554#discussion_r4080910820
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/IgniteCacheDatabaseSharedManager.java:
##########
@@ -1213,27 +1267,159 @@ public void ensureFreeSpaceForInsert(DataRegion
region, int dataRowSize) throws
// Note that not the whole page can be used to storing links,
// see PagesListNodeIO and PagesListMetaIO#getCapacity(), so we
pessimistically multiply the result on 1.5,
// in any way, the number of required pages is less than 1 percent.
- boolean oomThreshold = (memorySize / pageMem.systemPageSize()) <
+ boolean oomThreshold = (regCfg.getMaxSize() /
pageMem.systemPageSize()) <
((double)dataRowSize / pageMem.pageSize() + nonEmptyPages * (8.0 *
1.5 / pageMem.pageSize() + 1) + 256 /*one page per bucket*/);
- if (oomThreshold) {
- IgniteOutOfMemoryException oom = new
IgniteOutOfMemoryException("Out of memory in data region [" +
- "name=" + regCfg.getName() +
- ", initSize=" + U.readableSize(regCfg.getInitialSize(), false)
+
- ", maxSize=" + U.readableSize(regCfg.getMaxSize(), false) +
- ", persistenceEnabled=" + regCfg.isPersistenceEnabled() + "]
Try the following:" + U.nl() +
- " ^-- Increase maximum off-heap memory size
(DataRegionConfiguration.maxSize)" + U.nl() +
- " ^-- Enable Ignite persistence
(DataRegionConfiguration.persistenceEnabled)" + U.nl() +
- " ^-- Enable eviction or expiration policies"
- );
+ if (oomThreshold)
+ throw outOfMemory(regCfg);
+ }
+
+ /**
+ * Size-aware reserve for an eviction-enabled non-persistent region. Runs
eviction until the free list holds
+ * enough real empty pages to accommodate the row, or throws {@link
IgniteOutOfMemoryException} if the goal is
+ * unreachable / no progress can be made. Progress is measured against the
number of empty pages in the free list
+ * (the only resource a subsequent fragmented write can reliably consume
once the region is effectively full); the
+ * region's spare capacity (headroom) is only trusted in the fast path
while the region is below the eviction
+ * threshold.
+ *
+ * @param region Data region.
+ * @param regCfg Data region configuration.
+ * @param dataRowSize Size of data row to be inserted.
+ * @throws IgniteOutOfMemoryException If the target cannot be reached (row
too large for the region or eviction
+ * makes no progress).
+ * @throws IgniteCheckedException If failed to evict data pages.
+ */
+ private void ensureFreeSpaceForEviction(
+ DataRegion region,
+ DataRegionConfiguration regCfg,
+ int dataRowSize
+ ) throws IgniteOutOfMemoryException, IgniteCheckedException {
+ PageMemory pageMem = region.pageMemory();
+
+ // Maximum payload bytes that a single data page can hold for a
fragmented row.
+ long pagePayload = pageMem.pageSize() -
AbstractDataPageIO.MIN_DATA_PAGE_OVERHEAD;
- if (cctx.kernalContext() != null)
- cctx.kernalContext().failure().process(new
FailureContext(FailureType.CRITICAL_ERROR, oom));
+ // A row that fits into the steady-state empty-pages pool is satisfied
by normal threshold eviction, so the
+ // fast path is a single comparison (no page computation, free-list
lookup or page-memory reads on the hot
+ // small-put path).
+ if (dataRowSize <= regCfg.getEmptyPagesPoolSize() * pagePayload)
+ return;
- throw oom;
+ CacheFreeList freeList = freeListMap.get(regCfg.getName());
+
+ if (freeList == null)
+ return;
+
+ long totalPages = regCfg.getMaxSize() / pageMem.systemPageSize();
+
+ // Pages the row will actually occupy once written, and which the free
list must hand out on demand during
+ // the fragmented write.
+ long requiredPages = (dataRowSize + pagePayload - 1) / pagePayload;
+
+ // The row fundamentally cannot fit into the whole region.
+ if (requiredPages > totalPages)
+ throw outOfMemory(regCfg);
+
+ // The reserve must guarantee `requiredPages` REAL empty pages, not
just apparent headroom. Both are shared and
+ // non-exclusive (emptyDataPages() is a snapshot; any writer can
consume them), but once the region is full
+ // (loadedPages == totalPages) headroom can no longer grow it (fresh
allocateDataPage -> raw OOM), while empty
+ // pages in the reuse bucket stay reachable via takePage(). So empty
pages are the only resource the fragmented
+ // write can consume on a full region. The TOCTOU between this reserve
and the actual write is closed by the
+ // lazy re-reserve in AbstractFreeList#writeSinglePage.
+ long emptyPages = freeList.emptyDataPages();
+
+ // The gate reuses evictionThreshold as a regime boundary, not as
"when to start eviction" (evictionRequired()
+ // does that, stopping on emptyPages >= poolSize; no last 10% of page
memory is left unusable). Below the
+ // threshold the region has real slack, so a row fitting into the
combined spare space is satisfied without
+ // eviction (live, e.g. short-TTL, entries are not evicted just to
accumulate empty pages). At/above it headroom
+ // is no longer trustworthy (concurrent writers could commit the same
headroom - TOCTOU), so only real empty
+ // pages are counted and eviction is driven below.
+ boolean evictionRegime = pageMem.loadedPages() >= (long)(totalPages *
regCfg.getEvictionThreshold());
Review Comment:
The eviction in your scenario (10GB total, 1GB headroom, 500MB row) is
indeed excessive - 500MB of live entries are evicted even though 1GB of
headroom is available.
Part 1 of your suggestion (never use eviction below threshold) is already
implemented: the !evictionRegime branch in the fast path (line 1305) trusts
headroom below the threshold.
Part 2 (still consider headroom above threshold) is unsafe under contention:
when headroom is trusted in the reserve but later exhausted by a concurrent
writer, the re-reserve runs inside writeSinglePage → BPlusTree.invoke under an
entry lock. Eviction with tryLock=true skips locked entries, and if all
eviction candidates are locked by concurrent writers, eviction makes no
progress → OOM (RandomLruPageEvictionConcurrentWritesTest fails).
Two alternatives were tried:
1) removing the !evictionRegime gate;
2) partial eviction (toEvict = requiredPages - emptyPages - headroom) +
RE_RESERVE_ATTEMPTS=4.
Both fail with the same OOM.
The excessive eviction is the intentional cost of correctness under
contention. Eliminating it requires moving the size-aware reserve before
lockEntry(). But because this requires quite a few changes, I wanted to know
your opinion first - is it worth doing?
About the test: added `testLargeRowAboveThresholdEvictsAndSucceeds` verifies
the above-threshold path — the region is pre-filled near capacity, a 32 MiB row
triggers the size-aware eviction loop, isEvictionsStarted() becomes true, and
the write succeeds.
The region does not grow beyond the threshold in practice:
evictionRequired() in insertDataRows triggers normal threshold eviction when
loadedPages crosses the threshold, holding the region at the boundary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
