Conversation
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (18)
|
wernerdv
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves in-memory page eviction to support large rows and avoid lock-ordering deadlocks.
Changes:
- Adds size-aware eviction for single and batch inserts.
- Introduces non-blocking eviction locking and progress guards.
- Expands eviction regression tests for both LRU modes.
File summaries
| File | Description |
|---|---|
| modules/core/src/test/java/org/apache/ignite/testsuites/IgniteCacheEvictionSelfTestSuite.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/RandomLruPageEvictionWithExpiryPolicyTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/RandomLruPageEvictionSizeAwareTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/RandomLruPageEvictionConcurrentWritesTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/Random2LruPageEvictionWithExpiryPolicyTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/Random2LruPageEvictionSizeAwareTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/Random2LruPageEvictionConcurrentWritesTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionWithExpiryPolicyAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionSizeAwareAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionPutLargeObjectsAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionMetricTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionGuardOomTest.java | Updated as part of this pull request. |
| modules/core/src/test/java/org/apache/ignite/internal/processors/cache/eviction/paged/PageEvictionConcurrentWritesAbstractTest.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/RowStore.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/IgniteCacheDatabaseSharedManager.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/evict/PageAbstractEvictionTracker.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheMapEntry.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheEntryEx.java | Updated as part of this pull request. |
| modules/core/src/main/java/org/apache/ignite/configuration/DataRegionConfiguration.java | Updated as part of this pull request. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 11
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9ffb5d4 to
956bfc5
Compare
956bfc5 to
4d6ca55
Compare
2ba28e5 to
ad83772
Compare
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (18)
|
9c8c1c1 to
fb5b61e
Compare
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (18)
|
|
JmhPageEvictionBenchmark results on my local machine: master: branch: |
| // as a raw IgniteOutOfMemoryException (wrapped into CorruptedFreeListException in the batch path). | ||
| // | ||
| // The re-reserve is an inline demand-eviction: reached from the BPlusTree.invoke row-creation closure, it may | ||
| // re-entrantly remove other entries from the same data tree. That is safe because the closure runs with no |
There was a problem hiding this comment.
That is safe because the closure runs with no data-tree page locks held.
Are you sure about this statement?
As far as I know BPlusTree.invoke holds the write lock on leaf page while execution the closure, so it's not deadlock safe. Deadlock is possible between data-tree leaf pages since page can contain more than one entry (acquired entry lock is not enough protection).
Also, as far as I understand there can be deadlock on expiration: Expiration thread holds write lock on pending tree leaf page and reads data tree (with read lock). While addRow thread holds write lock on data tree leaf page and can concurrently evict data (remove rows) which remove ttl entries and requires write lock on pending tree leaf page.
There was a problem hiding this comment.
Partially correct.
The premise — "BPlusTree.invoke holds the leaf write lock while running the closure" — is not supported by the code: BPlusTree.invokeDown releases the leaf read lock when read(...) returns and takes the leaf write lock only afterwards (in tryInsert/tryReplace/tryRemoveFromLeaf). Your broader point stands: entry tryLock covers only entry-level ordering and does not make eviction page-level lock-free.
The cross-tree (data→pending vs pending→data) residual risk with the TTL worker is real and documented in the code comment; a full fix is out of scope.
Please correct me if I'm wrong.
There was a problem hiding this comment.
Looks like you are right about the cache data tree lock. But if lock is not held during invokeClosure, then pendingTree lock is also can't lead to deadlocks, so new comment is not correct. New eviction mechanism removes entries from cache data tree (with write lock-unlock), and after that removes pending tree entry (under write lock-unlock), and removes data entry (under write lock-unlock). If invokeClosure is not holds the lock, there are no nested locks and TTL path is safe too.
There was a problem hiding this comment.
Also statement removes entries with no data-tree page locks held can be read as locks held by removing entries, but here it means locks held by row-creation closure, please rephrase.
There was a problem hiding this comment.
Also statement removes entries with no data-tree page locks held can be read as locks held by removing entries, but here it means locks held by row-creation closure, please rephrase.
| */ | ||
| public void addRows(Collection<? extends CacheDataRow> rows, | ||
| IoStatisticsHolder statHolder) throws IgniteCheckedException { | ||
| public void addRows(Collection<? extends CacheDataRow> rows, IoStatisticsHolder statHolder) throws IgniteCheckedException { |
There was a problem hiding this comment.
addRows is only executed on rebalance. Why do we need this new code for rebalance but not for regular put?
reserving only the largest is insufficient
But you do exactly the same. Ensure free space for each row before any row is inserted, so only free space for largest row will be ensured. Other rows reservation will go through writeSinglePage.
There was a problem hiding this comment.
Two points correct, one not.
(1) Rebalance-only: correct — regular puts use addRow (per-put reserve), both paths covered.
(2) Per-row loop == reserve for the largest: correct — a single reserve for the max row is equivalent.
(3) "Other rows go through writeSinglePage" — actually the batch path uses insertDataRows, whose per-page protection now comes from the shared takePageWithReserve re-reserve; the pre-reserve bounds the empty-pages counter up front.
Updated comment.
There was a problem hiding this comment.
But why can't we prereserve the whole size and not rely to per-page reservation? What the profit of max entry size reservation?
There was a problem hiding this comment.
The primary advantage is that only the minimum required for the largest single row is evicted, not the entire batch upfront.
The reserve is non-exclusive (shared emptyDataPages counter), so even sum-reserve does not guarantee pages for this batch — concurrent writers can consume them, and the re-reserve in takePageWithReserve is still needed. WriteRowsHandler packs multiple small rows into one page, so sum(rowSizes) / pagePayload is a pessimistic upper bound: live entries are evicted for pages that packing would never use.
Max-row reserve evicts only what the largest row requires; the remainder is picked up per-page by the re-reserve in insertDataRows → takePageWithReserve — exactly at the point of need.
Under contention both approaches degrade to the same per-page re-reserve.
| */ | ||
| public void addRows(Collection<? extends CacheDataRow> rows, | ||
| IoStatisticsHolder statHolder) throws IgniteCheckedException { | ||
| public void addRows(Collection<? extends CacheDataRow> rows, IoStatisticsHolder statHolder) throws IgniteCheckedException { |
There was a problem hiding this comment.
But why can't we prereserve the whole size and not rely to per-page reservation? What the profit of max entry size reservation?
| // 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) |
There was a problem hiding this comment.
What if there are two concurrent puts with 60 pages each and only 100 pages left?
There was a problem hiding this comment.
With emptyPagesPoolSize=100, both puts hit the fast path (60 × pagePayload ≤ 100 × pagePayload → return without eviction). The first put consumes 60 of the 100 available pages. The second put finds only 40 left → takePage returns 0L → takePageWithReserve re-runs ensureFreeSpaceForInsert for the remaining 60 pages. If eviction can free 20 pages (evictable entries exist), the put succeeds. If not (region full, all candidates locked), ensureFreeSpaceForEviction throws OOM after the no-progress timeout.
| // 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()); |
There was a problem hiding this comment.
Still don't understand how it works.
Suppose we have 10Gb total page memory, 1Gb not allocated and no empty pages. We are trying to insert a 500Mb row. Here we have evictionRegime = true, in this case we only check emptyPages >= requiredPages (false) before eviction and stop to evict only when emptyPages >= requiredPages. So 500Mb will be evicted and we came to the same state: 1Gb is not allocated and no empty pages. How can we reuse top 10% of page memory in this case?
Perhaps we can use regCfg.getEvictionThreshold as fast path (never use eviction if we are below this point), but not as "rely only on empty pages after this point" flag.
There was a problem hiding this comment.
Any test for this behavior (that we can grow beyond EvictionThreshold)?
There was a problem hiding this 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:
- removing the !evictionRegime gate;
- 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.
| // 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()); |
There was a problem hiding this comment.
Any test for this behavior (that we can grow beyond EvictionThreshold)?
1f90e7d to
2abcca5
Compare
2abcca5 to
fcca5ab
Compare
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.