tkalkirill commented on code in PR #5328:
URL: https://github.com/apache/ignite-3/pull/5328#discussion_r1976995942
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/AbstractBplusTreePageMemoryTest.java:
##########
@@ -3116,117 +3108,66 @@ static void clearStaticResources() {
writeLocks.clear();
}
- /**
- * Constructor.
- *
- * @param delegate Real implementation of page lock listener.
- */
- private TestPageLockListener(PageLockListener delegate) {
- this.delegate = delegate;
- }
-
/** {@inheritDoc} */
Review Comment:
Maybe get rid of it? and others?
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/inmemory/ItBplusTreeVolatilePageMemoryTest.java:
##########
@@ -58,7 +65,8 @@ protected PageMemory createPageMemory() {
return new VolatilePageMemory(
(VolatilePageMemoryProfileConfiguration)
fixConfiguration(storageProfileConfiguration),
ioRegistry,
- PAGE_SIZE
+ PAGE_SIZE,
+ wrapLock(new OffheapReadWriteLock(128))
Review Comment:
same
##########
modules/page-memory/src/testFixtures/java/org/apache/ignite/internal/pagememory/util/PageLockListener.java:
##########
@@ -27,60 +27,46 @@ public interface PageLockListener extends ManuallyCloseable
{
/**
* Callback that's called before write lock acquiring.
*
- * @param groupId Group ID.
- * @param pageId Page ID.
- * @param page Page pointer.
+ * @param lock Lock pointer.
*/
- void onBeforeWriteLock(int groupId, long pageId, long page);
+ void onBeforeWriteLock(long lock);
Review Comment:
Please rename to lockPtr below and heirs.
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/freelist/FreeListImplTest.java:
##########
@@ -178,7 +177,8 @@ private PageMemory createPageMemory(int pageSize) throws
Exception {
return new VolatilePageMemory(
(VolatilePageMemoryProfileConfiguration)
fixConfiguration(storageProfileCfg),
ioRegistry,
- pageSize
+ pageSize,
+ new OffheapReadWriteLock(128)
Review Comment:
Same about constant
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/AbstractBplusTreePageMemoryTest.java:
##########
@@ -3116,117 +3108,66 @@ static void clearStaticResources() {
writeLocks.clear();
}
- /**
- * Constructor.
- *
- * @param delegate Real implementation of page lock listener.
- */
- private TestPageLockListener(PageLockListener delegate) {
- this.delegate = delegate;
- }
-
/** {@inheritDoc} */
@Override
- public void onBeforeReadLock(int cacheId, long pageId, long page) {
- delegate.onBeforeReadLock(cacheId, pageId, page);
-
- if (PRINT_LOCKS) {
- println(" onBeforeReadLock: " + hexLong(pageId));
- }
-
- assertNull(beforeReadLock.put(threadId(), pageId));
+ public void onBeforeReadLock(long lock) {
+ assertNull(beforeReadLock.put(threadId(), lock));
}
/** {@inheritDoc} */
@Override
- public void onReadLock(int cacheId, long pageId, long page, long
pageAddr) {
- delegate.onReadLock(cacheId, pageId, page, pageAddr);
-
- if (PRINT_LOCKS) {
- println(" onReadLock: " + hexLong(pageId));
- }
-
- if (pageAddr != 0L) {
- long actual = getPageId(pageAddr);
-
- checkPageId(pageId, pageAddr);
+ public void onReadLock(long lock, boolean locked) {
+ if (locked) {
+ long actual = getPageId(lock - lockOffset);
Review Comment:
It could be formalized into a method, at your discretion.
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/AbstractBplusTreePageMemoryTest.java:
##########
@@ -3265,15 +3206,6 @@ protected static void println(@Nullable String msg) {
System.out.println(msg);
Review Comment:
Holy cow, it's time to change it to logging.
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/persistence/ItBplusTreeReuseListPersistentPageMemoryTest.java:
##########
@@ -71,7 +77,7 @@ protected PageMemory createPageMemory() throws Exception {
mockCheckpointTimeoutLock(true),
() -> null,
PAGE_SIZE,
- new OffheapReadWriteLock(128)
+ wrapLock(new OffheapReadWriteLock(128))
Review Comment:
Same about constant
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/persistence/ItBplusTreePersistentPageMemoryTest.java:
##########
@@ -66,6 +67,11 @@ public class ItBplusTreePersistentPageMemoryTest extends
AbstractBplusTreePageMe
private OffheapReadWriteLock offheapReadWriteLock;
+ @BeforeAll
+ static void initLockOffset() {
+ lockOffset = PersistentPageMemory.PAGE_LOCK_OFFSET;
+ }
+
/** {@inheritDoc} */
Review Comment:
Maybe get rid of?
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/inmemory/ItBplusTreeReuseListVolatilePageMemoryTest.java:
##########
@@ -55,7 +62,8 @@ protected PageMemory createPageMemory() {
return new VolatilePageMemory(
(VolatilePageMemoryProfileConfiguration)
fixConfiguration(storageProfileCfg),
ioRegistry,
- PAGE_SIZE
+ PAGE_SIZE,
+ wrapLock(new OffheapReadWriteLock(128))
Review Comment:
same about constant
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/ItBplusTreeReplaceRemoveRaceTest.java:
##########
@@ -103,7 +103,8 @@ protected PageMemory createPageMemory() throws Exception {
return new VolatilePageMemory(
(VolatilePageMemoryProfileConfiguration)
fixConfiguration(dataRegionCfg),
ioRegistry,
- 512
+ 512,
+ new OffheapReadWriteLock(128)
Review Comment:
Let's not use magic constants. Let's make a variable with a clear name and
description.
It would be great if the same thing happened with the value above.
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/datastructure/DataStructure.java:
##########
@@ -446,6 +439,6 @@ protected int pageSize() {
*/
@Override
public void close() {
- lockLsnr.close();
+ // No-op.
Review Comment:
Maybe get rid of it? Make the class abstract and let the heirs implement it.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/VolatilePageMemoryDataRegion.java:
##########
@@ -72,7 +76,12 @@ public class VolatilePageMemoryDataRegion implements
DataRegion<VolatilePageMemo
* Starts the in-memory data region.
*/
public void start() {
- VolatilePageMemory pageMemory = new VolatilePageMemory(cfg,
ioRegistry, pageSize);
+ int lockConcLvl = IgniteSystemProperties.getInteger(
+ IGNITE_OFFHEAP_LOCK_CONCURRENCY_LEVEL,
+
Integer.highestOneBit(Runtime.getRuntime().availableProcessors() * 4)
Review Comment:
Why is the default value so high, please write a comment about it.****
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/inmemory/VolatilePageMemoryNoLoadSelfTest.java:
##########
@@ -53,7 +54,8 @@ protected VolatilePageMemory memory() {
return new VolatilePageMemory(
(VolatilePageMemoryProfileConfiguration)
fixConfiguration(storageProfileCfg),
ioRegistry,
- PAGE_SIZE
+ PAGE_SIZE,
+ new OffheapReadWriteLock(128)
Review Comment:
same about constant
##########
modules/page-memory/src/testFixtures/java/org/apache/ignite/internal/pagememory/util/ListeningOffheapReadWriteLock.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.pagememory.util;
+
+import org.apache.ignite.internal.util.OffheapReadWriteLock;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Adds a {@link PageLockListener} to an {@link OffheapReadWriteLock}.
+ */
+public class ListeningOffheapReadWriteLock extends OffheapReadWriteLock {
Review Comment:
The variable name `lock` is not very obvious, I would ask you to change it.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/VolatilePageMemoryDataRegion.java:
##########
@@ -72,7 +76,12 @@ public class VolatilePageMemoryDataRegion implements
DataRegion<VolatilePageMemo
* Starts the in-memory data region.
*/
public void start() {
- VolatilePageMemory pageMemory = new VolatilePageMemory(cfg,
ioRegistry, pageSize);
+ int lockConcLvl = IgniteSystemProperties.getInteger(
+ IGNITE_OFFHEAP_LOCK_CONCURRENCY_LEVEL,
+
Integer.highestOneBit(Runtime.getRuntime().availableProcessors() * 4)
+ );
+
+ VolatilePageMemory pageMemory = new VolatilePageMemory(cfg,
ioRegistry, pageSize, new OffheapReadWriteLock(lockConcLvl));
Review Comment:
```suggestion
var pageMemory = new VolatilePageMemory(cfg, ioRegistry, pageSize,
new OffheapReadWriteLock(lockConcLvl));
```
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/mv/BlobStorageTest.java:
##########
@@ -80,7 +81,11 @@ void createStorage() {
};
pageMemory = spy(new VolatilePageMemory(
- (VolatilePageMemoryProfileConfiguration)
fixConfiguration(dataRegionCfg), pageIoRegistry, PAGE_SIZE));
+ (VolatilePageMemoryProfileConfiguration)
fixConfiguration(dataRegionCfg),
+ pageIoRegistry,
+ PAGE_SIZE,
+ new OffheapReadWriteLock(128)
Review Comment:
Same about constant
##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/persistence/ItBplusTreeReuseListPersistentPageMemoryTest.java:
##########
@@ -48,6 +49,11 @@ public class ItBplusTreeReuseListPersistentPageMemoryTest
extends AbstractBplusT
)
private StorageProfileConfiguration dataRegionCfg;
+ @BeforeAll
+ static void initLockOffset() {
+ lockOffset = PersistentPageMemory.PAGE_LOCK_OFFSET;
+ }
+
/** {@inheritDoc} */
Review Comment:
Same
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
