alex-plekhanov commented on code in PR #13554:
URL: https://github.com/apache/ignite/pull/13554#discussion_r4386234883
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -493,6 +576,11 @@ public AbstractFreeList(
rmvRow = new RemoveRowHandler(cacheGrpId == 0);
this.evictionTracker = dataRegion.evictionTracker();
+
+ reserveEnabled = !dataRegion.config().isPersistenceEnabled() &&
Review Comment:
`ctx.cache().context().database().isReserveEnabled(dataRegion)`?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -1134,8 +1257,194 @@ public int emptyDataPages() {
}
}
+ /** {@inheritDoc} */
+ @Override public long reserve(long rowSize, boolean bestEffort) throws
IgniteOutOfMemoryException, IgniteCheckedException {
+ if (!reserveEnabled)
+ return 0;
+
+ long claimPages = requiredPages(rowSize);
+
+ if (!fitsRegion(claimPages)) {
+ if (bestEffort)
+ return 0; // Let the page memory decide.
+
+ throw outOfMemory();
+ }
+
+ long limitPages = limitPages(claimPages);
+
+ boolean recentStall = bestEffort && U.currentTimeMillis() -
lastStallMillis < EVICTION_NO_PROGRESS_TIMEOUT_MILLIS;
+
+ long backoffNanos = EVICTION_BACKOFF_START_NANOS;
+
+ long lastProgressNanos = System.nanoTime();
Review Comment:
Why do we need nanos here? This value is only compared with
EVICTION_NO_PROGRESS_TIMEOUT_MILLIS, so let's use cached
`U.currentTimeMillis()` and avoid `System.nanoTime()` for optimization purposes.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -1134,8 +1257,194 @@ public int emptyDataPages() {
}
}
+ /** {@inheritDoc} */
+ @Override public long reserve(long rowSize, boolean bestEffort) throws
IgniteOutOfMemoryException, IgniteCheckedException {
+ if (!reserveEnabled)
+ return 0;
+
+ long claimPages = requiredPages(rowSize);
+
+ if (!fitsRegion(claimPages)) {
+ if (bestEffort)
+ return 0; // Let the page memory decide.
+
+ throw outOfMemory();
+ }
+
+ long limitPages = limitPages(claimPages);
+
+ boolean recentStall = bestEffort && U.currentTimeMillis() -
lastStallMillis < EVICTION_NO_PROGRESS_TIMEOUT_MILLIS;
+
+ long backoffNanos = EVICTION_BACKOFF_START_NANOS;
+
+ long lastProgressNanos = System.nanoTime();
+
+ while (true) {
+ long reserved = reservedPages.get();
+ long deficit = deficit(claimPages, limitPages, reserved);
+
+ if (deficit <= 0) {
+ if (reservedPages.compareAndSet(reserved, reserved +
claimPages))
+ return claimPages;
+
+ continue;
+ }
+
+ if (memMetrics.onPageEvictionsStarted()) {
+ U.warn(log, "Page-based evictions started." +
+ " Consider increasing 'maxSize' on Data Region
configuration: " + dataRegion.config().getName());
+ }
+
+ evictionTracker.evictDataPage();
+
+ memMetrics.updateEvictionRate();
+
+ if (deficit(claimPages, limitPages, reservedPages.get()) <
deficit) {
+ lastProgressNanos = System.nanoTime();
+
+ backoffNanos = EVICTION_BACKOFF_START_NANOS;
+ }
+ else if (recentStall)
+ return 0;
+ else if (System.nanoTime() - lastProgressNanos >
TimeUnit.MILLISECONDS.toNanos(EVICTION_NO_PROGRESS_TIMEOUT_MILLIS)) {
+ if (!bestEffort)
+ throw outOfMemory();
+
+ lastStallMillis = U.currentTimeMillis();
+
+ LT.warn(log, "Page eviction made no progress, the row is
inserted without a page reservation " +
+ "[region=" + dataRegion.config().getName() + ", timeout="
+ EVICTION_NO_PROGRESS_TIMEOUT_MILLIS + "ms]");
+
+ return 0;
+ }
+ else {
+ LockSupport.parkNanos(backoffNanos);
+
+ backoffNanos = Math.min(backoffNanos << 1,
EVICTION_BACKOFF_MAX_NANOS);
+ }
Review Comment:
Looks like this block can be moved upper, to avoid double `deficit`
invocation on each iteration. For example, something like this:
```
long deficit = deficit(claimPages, limitPages, reserved);
if (prevDeficit > 0) {
if (deficit < prevDeficit) {
...
}
}
prevDeficit = deficit;
```
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheMapEntry.java:
##########
@@ -2731,6 +2778,8 @@ protected long nextPartitionCounter(IgniteInternalTx tx,
@Nullable Long updateCn
}
finally {
unlockEntry();
+
+ releaseReserved(reserved);
Review Comment:
Looks inconsistent, reservation acquired externally, but released inside
method. `versionedValue` used only once with reservation and once without
reservation, maybe it's worth to leave method as is and surround with
reserve/release when reservation is required.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridDhtAtomicCache.java:
##########
@@ -2978,6 +2986,52 @@ else if (GridDhtCacheEntry.ReaderId.contains(readers,
nearNode.id())) {
}
}
+ /**
+ * Reserves free-list pages for the <b>total</b> size of all rows in the
batch: size-aware admission control for
+ * the primary atomic path, which acquires entry locks via {@link
#lockEntries} before {@code innerUpdate} and so
+ * cannot reserve (or evict) per entry there. Reserving before {@link
#lockEntries} makes room for the data pages
+ * of all rows of a batch with known value sizes, so their inserts are not
expected to hit an out-of-memory in
+ * {@code FreeList.insertDataRow} (pages outside the reservation, e.g.
B+tree pages, are covered by the empty pages
+ * pool).
+ * <p>
+ * The full row size is reserved for updates too: an update inserts the
new row before removing the old one, unless
+ * it is done in place (same row size, no SQL indexes, unchanged expire
time), which can't be known before the entry
+ * lock. For in-place updates near the eviction threshold this keeps extra
empty pages free (up to the size of the
+ * rows being written concurrently) instead of using them for data; it is
not a correctness issue.
+ *
+ * @param req Update request.
+ * @return Reserved pages to release via {@code releaseInRegion}, or
{@code 0} if reservation is not engaged.
+ * @throws IgniteCheckedException If failed.
+ */
+ private long reserveBatchSpace(GridNearAtomicAbstractUpdateRequest req)
throws IgniteCheckedException {
+ DataRegion dataRegion = ctx.dataRegion();
+
+ // Skip computing the row sizes (it may require marshalling) if the
reservation is not engaged for the region.
+ if (!ctx.shared().database().isReserveEnabled(dataRegion))
+ return 0;
+
+ CacheObjectValueContext coctx = ctx.cacheObjectContext();
+
+ boolean storeCacheId = ctx.group().storeCacheIdInDataPage();
+
+ long sumRowSize = 0;
+
+ for (int i = 0; i < req.size(); i++) {
+ KeyCacheObject key = req.key(i);
+
+ CacheObject val = req.writeValue(i);
Review Comment:
For invoke we reserve space for entry processor here instead of new value.
For delete, also key size + some overhead is reserved.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheMapEntry.java:
##########
@@ -1445,8 +1453,14 @@ else if (log.isDebugEnabled())
AtomicCacheUpdateClosure c;
+ long reserved = 0;
+
if (!primary && !isNear())
- ensureFreeSpace();
+ // Best-effort: a refused backup update is not safe, the primary
has already applied it.
+ reserved = reserveEntrySpace(
Review Comment:
When operation is DELETE or TRANSFORM with null new value, this operation is
also reserves space for key + some overhead, but shouldn't.
--
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]
