IGNITE-29032 Improvement of the page eviction mechanism for in-memory - #13554
IGNITE-29032 Improvement of the page eviction mechanism for in-memory#13554wernerdv wants to merge 13 commits into
Conversation
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (18)
|
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: |
| * while the calling thread already holds entry locks. When set, entries whose locks are contended are skipped | ||
| * (via a non-blocking {@code evictInternal}) instead of blocking, avoiding a lock-ordering deadlock. | ||
| */ | ||
| private static final ThreadLocal<Boolean> EVICT_NON_BLOCKING = new ThreadLocal<>(); |
There was a problem hiding this comment.
It's strange to use thread-local to pass parameter to next stack level. Let's use
evictDataPage(boolean blocking) -> evictDataPage(int pageIdx, boolean blocking)
Instead of
evictDataPageNonBlocking -> set thread-local -> evictDataPage() -> evictDataPage(int pageIdx) -> get thread local.
There was a problem hiding this comment.
Agreed.
Moved the capability into the interface as evictDataPage(boolean tryLock) (default evictDataPage() delegates to false), removed the ThreadLocal.
As a follow-up the method now returns boolean so the size-aware progress guard can count an actual eviction as progress. Parameter named tryLock (matching evictInternal(..., tryLock) / lockEntry(boolean tryLock)) rather than blocking.
| // 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.
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.