dschneider-pivotal commented on a change in pull request #7407:
URL: https://github.com/apache/geode/pull/7407#discussion_r827251656
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
##########
@@ -55,6 +57,16 @@
public static final String FREE_OFF_HEAP_MEMORY_PROPERTY =
GeodeGlossary.GEMFIRE_PREFIX + "free-off-heap-memory";
+ public static final int UPDATE_OFF_HEAP_STATS_FREQUENCY_MS =
+ SystemProperty.getProductIntegerProperty(
+ "update-off-heap-stats-frequency-ms").orElse(360000);
+
+ /**
+ * Can be overridden during testing.
+ */
+ @MakeNotStatic
+ private static volatile int updateOffHeapStatsFrequencyMs =
UPDATE_OFF_HEAP_STATS_FREQUENCY_MS;
Review comment:
We really don't want to add new code that needs the MakeNotStatic
annotation.
I think you can make these two fields non-static and this one can be final
instead of volatile. Just have them be initialized each time a
MemoryAllocatorImpl is created. The product only does this when the cache is
created. In you test that wants to set it to 100 just add support to
basicCreateOffHeapStorage so it can be passed in to the MemoryAllocatorImpl
constructor. In you other test that sets it to 1000 and then creates the cache,
use the system property to set it. A helper class exists that will
automatically unset sysprops that the unit test has set.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
##########
@@ -139,9 +153,14 @@ private static MemoryAllocatorImpl
create(OutOfOffHeapMemoryListener ooohml,
singleton = result;
LifecycleListener.invokeAfterCreate(result);
created = true;
+ singleton.updateStatsTask = createUpdateFreeListStatsThread();
+ singleton.updateStatsTask.start();
}
} finally {
if (!created) {
+ if (singleton != null && singleton.updateStatsTask != null) {
Review comment:
Since updateStatsTask is not initialized until after "created" is set to
true, these three lines are dead code and should be removed.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/OffHeapStoredObjectAddressStack.java
##########
@@ -48,17 +50,24 @@ public void offer(long e) {
synchronized (this) {
OffHeapStoredObject.setNext(e, topAddr);
topAddr = e;
+ length++;
}
}
+ public int getLength() {
Review comment:
since this class represents a "stack" I think "size" would be better
than "length". In Java collections like this usually use size and length is
used for arrays and strings.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
##########
@@ -360,6 +394,9 @@ public void close() {
try {
LifecycleListener.invokeBeforeClose(this);
} finally {
+ if (updateStatsTask != null) {
Review comment:
realClose should be changed to have a call of shutdown on your new
"final" ScheduledExecutorService. You could keep a reference to the
ScheduledFuture returned by scheduleWithFixedDelay and call cancel() on it
here. The complication is that we don't always return the offheap memory to the
OS to prevent SEGV and this can cause us to reuse that offheap memory when
doing a reconnect. See the MemoryAllocatorImpl.reuse method
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/OffHeapStoredObjectAddressStack.java
##########
@@ -27,6 +27,8 @@
// Ok to read without sync but must be synced on write
private volatile long topAddr;
+ private volatile int length = 0;
Review comment:
add the same comments on this field that is on topAddr
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
##########
@@ -139,9 +153,14 @@ private static MemoryAllocatorImpl
create(OutOfOffHeapMemoryListener ooohml,
singleton = result;
LifecycleListener.invokeAfterCreate(result);
created = true;
+ singleton.updateStatsTask = createUpdateFreeListStatsThread();
+ singleton.updateStatsTask.start();
}
} finally {
if (!created) {
+ if (singleton != null && singleton.updateStatsTask != null) {
Review comment:
This finally block is really meant to handle a problem with allocating
the off heap memory. Once we get to the point of calling the
MemoryAllocatorImpl constructor we didn't expect any exceptions from the
constructor or invokeAfterCreate. I don't think you need to be concerned here
with cleaning up the new stat thread
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
##########
@@ -55,6 +57,16 @@
public static final String FREE_OFF_HEAP_MEMORY_PROPERTY =
GeodeGlossary.GEMFIRE_PREFIX + "free-off-heap-memory";
+ public static final int UPDATE_OFF_HEAP_STATS_FREQUENCY_MS =
+ SystemProperty.getProductIntegerProperty(
+ "update-off-heap-stats-frequency-ms").orElse(360000);
Review comment:
I think "off-heap-stats-update-frequency-ms" reads a bit better. It
helps connect "frequency" to "update"
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/OffHeapStorage.java
##########
@@ -382,6 +393,12 @@ public void setFragmentation(int value) {
stats.setInt(fragmentationId, value);
}
+ @Override
+ public void setFreedChunks(int value) {
+ this.stats.setInt(freedChunksId, value);
Review comment:
this should be "setLong" not "setInt".
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
##########
@@ -153,6 +172,22 @@ private static MemoryAllocatorImpl
create(OutOfOffHeapMemoryListener ooohml,
return result;
}
+ private static Thread createUpdateFreeListStatsThread() {
+ return new Thread(() -> {
+ while (true) {
+ if (singleton != null) {
+ singleton.freeList.updateNonRealTimeStats();
Review comment:
you should not use "singleton" here. Instead just use "this" which will
never be null. Also instead of accessing freeList here just have a
updateNonRealTimeStats method on MemoryAllocatorImpl. It can then decide to
delegate to freeList.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/FreeListManager.java
##########
@@ -518,10 +520,58 @@ boolean doDefragment(int chunkSize) {
ma.getStats().setLargestFragment(largestFragment);
ma.getStats().setFragments(tmp.size());
ma.getStats().setFragmentation(getFragmentation());
+ ma.getStats().setFreedChunks(0);
return result;
}
+ public void updateNonRealTimeStats() {
+ ma.getStats().setLargestFragment(getLargestFragment());
+ ma.getStats().setFreedChunks(getFreedChunks());
+ }
+
+ public int getFreedChunks() {
+ int elementCountFromTinyFreeLists =
+ getElementCountFromTinyFreeLists();
+ int elementCountFromHugeFreeLists =
+ getElementCountFromHugeFreeLists();
+
+ return elementCountFromTinyFreeLists + elementCountFromHugeFreeLists;
+ }
+
+ private int getElementCountFromTinyFreeLists() {
+ int fragmentCount = 0;
+ for (int i = 0; i < tinyFreeLists.length(); i++) {
+ OffHeapStoredObjectAddressStack cl = tinyFreeLists.get(i);
+ if (cl != null) {
+ int length = cl.getLength();
+ fragmentCount += length;
+ }
+ }
+ return fragmentCount;
+ }
+
+ private int getElementCountFromHugeFreeLists() {
+ return hugeChunkSet.size();
+ }
+
+ private int getLargestFragment() {
+ int largestFreeSpaceFromFragments = 0;
+ for (Fragment f : fragmentList) {
+ int offset = f.getFreeIndex();
+ int diff = f.getSize() - offset;
+ if (diff < OffHeapStoredObject.MIN_CHUNK_SIZE) {
Review comment:
I think you should just get rid of this "if" block that compares to
MIN_CHUNK_SIZE. The next "if" that checks to see if "diff" is greater is all
you need. We wouldn't want this code to possibly blow up if assertions are
enabled in a production system (because of the "assert" being used). Also
instead of calling getFreeIndex and getSize just use Fragment.freeSpace().
If you are concerned with this method returning a value < MIN_CHUNK_SIZE but
> 0 then at the very end when you return a value you could say if it is <
MIN_CHUNK_SIZE then return 0.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java
##########
@@ -153,6 +172,22 @@ private static MemoryAllocatorImpl
create(OutOfOffHeapMemoryListener ooohml,
return result;
}
+ private static Thread createUpdateFreeListStatsThread() {
+ return new Thread(() -> {
Review comment:
Use LoggingThread(String, Runnable) here. This will make it a daemon
thread which I think we want here and it will make sure any unhandled exception
will get logged.
Even better use LoggingExecutors.newSingleThreadScheduledExecutor(String).
You could make this call in the MemoryAllocatorImpl constructor and the
result of the call should be stored in a final instance variable.
Use scheduleAtFixedRate to periodically call updateNonRealTimeStats. This
gets rid of having your own sleep code.
--
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...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
