alex-plekhanov commented on code in PR #13554:
URL: https://github.com/apache/ignite/pull/13554#discussion_r4097017925
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -615,7 +616,15 @@ private long allocateDataPage(int part) throws
IgniteCheckedException {
* max), so a fresh {@code allocateDataPage} could no longer grow it.
*/
private boolean regionEffectivelyFull() {
- return pageMem.loadedPages() >= dataRegion.config().getMaxSize() /
pageMem.systemPageSize();
+ long maxPages = dataRegion.config().getMaxSize() /
pageMem.systemPageSize();
+
+ // Each of up to 16 segments loses up to one page to allocation
overhead (lastAllocatedIdxPtr + alignment),
+ // so the theoretical max is never reached in practice. Subtract the
worst-case segment loss to get an
+ // effective limit that reflects real product scenarios.
+ if (maxPages > 16)
Review Comment:
Let's use SEG_CNT instead of hardcoded value
##########
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)
Review Comment:
> The second put finds only 40 left → takePage returns 0L →
takePageWithReserve re-runs ensureFreeSpaceForInsert for the remaining 60
pages.
As far as I understand, both entries will call ensureFreeSpace() and evict
up to 100 pages before add data. After that only size-aware eviction is
possible on takePageWithReserve. If first entry inserts 60 pages, and second
entry insert 40 pages, there are no pages left in page memory and
ensureFreeSpaceForInsert for next page will be called. But this method don't
even try to evict something, since it's quit on this condition (only 20 pages
left to insert and it's less than getEmptyPagesPoolSize). So OOM will be thown.
Did I miss something?
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionConcurrentWritesAbstractTest.java:
##########
@@ -19,86 +19,28 @@
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicLong;
-import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.ignite.IgniteCache;
-import org.apache.ignite.cache.affinity.rendezvous.RendezvousAffinityFunction;
-import org.apache.ignite.configuration.CacheConfiguration;
-import org.apache.ignite.configuration.DataRegionConfiguration;
import org.apache.ignite.configuration.DataStorageConfiguration;
import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.IgniteEx;
-import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
import org.junit.Test;
-import static
org.apache.ignite.configuration.DataStorageConfiguration.DFLT_PAGE_SIZE;
-
-/**
- * Concurrent deadlock test for size-aware page eviction.
- * <p>
- * The region is first filled with a large number of small entries (so there
is plenty of evictable page space), then
- * several threads concurrently insert large rows (larger than the empty-pages
pool). Each large insert goes through
- * the size-aware reserve and, for the single-row path, eviction under the new
entry lock with the non-blocking
- * {@code tryLockEntry}. The average data volume is kept within the region
capacity, so eviction frees already-stored
- * small entries rather than overrunning the free list. The test asserts that
no deadlock occurs (all threads finish
- * within a global deadline).
- */
-public abstract class PageEvictionConcurrentWritesAbstractTest extends
GridCommonAbstractTest {
- /** Off-heap region size. */
- private static final int SIZE = 256 * 1024 * 1024;
-
- /** Partition count (kept low so that index-tree structures do not exhaust
the region). */
- private static final int PARTITIONS = 32;
-
- /** Large record size (larger than the empty-pages pool so that each write
is size-aware). */
- private static final int LARGE_RECORD_SIZE = 2 * 1024 * 1024;
-
- /** Small record size used to pre-fill the region with evictable data. */
- private static final int SMALL_RECORD_SIZE = 4096;
-
- /** Empty pages pool size. */
- private static final int POOL_SIZE = 100;
-
- /** Number of small pre-fill entries, leaving a buffer that is exceeded by
the total of the large writes, so that
- * the last of them can only be stored by freeing pages via size-aware
eviction. The large records are small
- * enough that concurrent size-aware eviction reliably frees the required
pages (no spurious guard OOM). */
- private static final int SMALL_ENTRIES = 48_000;
-
- /** Number of writer threads. */
- private static final int THREADS = 2;
-
- /** Large rows inserted per thread. Their total (threads x rows) exceeds
the buffer left by the pre-fill, so the
- * last large writes overflow the region and require size-aware eviction
to free small entry pages. */
- private static final int LARGE_ROWS_PER_THREAD = 20;
-
- /** Global deadline for the whole test (protects against a
deadlock/busy-spin hang). */
- private static final long DEADLINE = TimeUnit.MINUTES.toMillis(3);
-
+/** Concurrent deadlock test for size-aware page eviction. */
+public abstract class PageEvictionConcurrentWritesAbstractTest extends
PageEvictionAbstractTest {
/** {@inheritDoc} */
@Override protected IgniteConfiguration getConfiguration(String gridName)
throws Exception {
- return super.getConfiguration(gridName)
- .setDataStorageConfiguration(new DataStorageConfiguration()
- .setDefaultDataRegionConfiguration(new
DataRegionConfiguration()
- .setInitialSize(SIZE)
- .setMaxSize(SIZE)
- .setEmptyPagesPoolSize(POOL_SIZE))
- .setPageSize(DFLT_PAGE_SIZE));
+ return
super.getConfiguration(gridName).setDataStorageConfiguration(new
DataStorageConfiguration());
Review Comment:
Now default data region size is used, eviction not even started.
Let's add `assertTrue(metrics.isEvictionsStarted());` to ensure that we
check something in this test.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionWithExpiryPolicyAbstractTest.java:
##########
@@ -115,85 +64,54 @@ private IgniteCache<Integer, Object> createCache(IgniteEx
ignite, String cacheNa
public void testLargePutWithExpiryNoDeadlock() throws Exception {
Review Comment:
We can't check if there is no deadlocks if there is only one thread and no
concurrent eviction
##########
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:
Part 1: It's not the same, logic under `!evictionRegime`: rely not only on
empty pages but on headroom too.
Part 2: For me it's still no clear why there is a difference between empty
pages and headroom in therms of safety under contention. Both are consumed
concurrently and looks like only reason why concurrent tests are passed, is
because 10% of memory remain unused (and it also can fail with certain level of
concurrency). But when you disable this heuristic (allow to allocate up to
total memory) it fails faster, just because safety buffer is smaller. Maybe
real reason for failures is something else (like condition above with
`getEmptyPagesPoolSize` check).
About test: I mean it should be checked that we can consume, for example,
95% of memory.
Naive check shows that we can't:
```
for (int i = 0; i < SMALL_ENTRIES * 2; i++)
cache.put(i, small);
DataRegion dr =
ignite.context().cache().context().database().dataRegion(null);
System.out.println(dr.pageMemory().loadedPages()); // 29320
System.out.println(SIZE / dr.pageMemory().systemPageSize()); // 32577
```
--
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]
