[GitHub] [geode] dschneider-pivotal commented on a diff in pull request #7715: GEODE-10323: Remove schedule threads in MemoryAllocatorImpl constructor

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on code in PR #7715:
URL: https://github.com/apache/geode/pull/7715#discussion_r904053762


##########
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java:
##########
@@ -98,20 +95,17 @@ public static MemoryAllocatorImpl getAllocator() {
 
   public static MemoryAllocator create(OutOfOffHeapMemoryListener ooohml, OffHeapMemoryStats stats,
       int slabCount, long offHeapMemorySize, long maxSlabSize,
-      int updateOffHeapStatsFrequencyMs) {
+      Supplier<Integer> updateOffHeapStatsFrequencyMsSupplier,
+      Supplier<NonRealTimeStatsUpdater> nonRealTimeStatsUpdaterSupplier) {
     return create(ooohml, stats, slabCount, offHeapMemorySize, maxSlabSize, null,
-        SlabImpl::new, updateOffHeapStatsFrequencyMs);
+        SlabImpl::new, updateOffHeapStatsFrequencyMsSupplier, nonRealTimeStatsUpdaterSupplier);
   }
 
-  public static MemoryAllocator create(OutOfOffHeapMemoryListener ooohml, OffHeapMemoryStats stats,
-      int slabCount, long offHeapMemorySize, long maxSlabSize) {
-    return create(ooohml, stats, slabCount, offHeapMemorySize, maxSlabSize, null,
-        SlabImpl::new, UPDATE_OFF_HEAP_STATS_FREQUENCY_MS);
-  }
-
-  private static MemoryAllocatorImpl create(OutOfOffHeapMemoryListener ooohml,
+  static MemoryAllocatorImpl create(OutOfOffHeapMemoryListener ooohml,
       OffHeapMemoryStats stats, int slabCount, long offHeapMemorySize, long maxSlabSize,
-      Slab[] slabs, SlabFactory slabFactory, int updateOffHeapStatsFrequencyMs) {
+      Slab[] slabs, SlabFactory slabFactory,
+      Supplier<Integer> updateOffHeapStatsFrequencyMsSupplier,

Review Comment:
   It seems like the update frequency "int" should be part of the NonRealTimeStatsUpdater and then you wouldn't need two suppliers.



##########
geode-core/src/main/java/org/apache/geode/internal/offheap/MemoryAllocatorImpl.java:
##########
@@ -239,11 +230,19 @@ private MemoryAllocatorImpl(final OutOfOffHeapMemoryListener oooml,
     this.stats.incMaxMemory(freeList.getTotalMemory());
     this.stats.incFreeMemory(freeList.getTotalMemory());
 
-    updateNonRealTimeStatsExecutor =
-        LoggingExecutors.newSingleThreadScheduledExecutor("Update Freelist Stats thread");
-    updateNonRealTimeStatsFuture =
-        updateNonRealTimeStatsExecutor.scheduleAtFixedRate(freeList::updateNonRealTimeStats, 0,
-            updateOffHeapStatsFrequencyMs, TimeUnit.MILLISECONDS);
+    this.updateOffHeapStatsFrequencyMs = updateOffHeapStatsFrequencyMs;
+
+    if (nonRealTimeStatsUpdaterSupplier == null) {
+      nonRealTimeStatsUpdater = new NonRealTimeStatsUpdater(freeList::updateNonRealTimeStats);
+    } else {
+      nonRealTimeStatsUpdater = nonRealTimeStatsUpdaterSupplier.get();
+    }
+  }
+
+  void start() {
+    if (nonRealTimeStatsUpdater != null) {

Review Comment:
   In the constructor above it looks like you always initialize nonRealTimeStatsUpdaterSupplier to non-null. So what good are these null checks? Oh I see. Your supplier returns null. That works but I think it would be better to have the supplier returns a dummy NonRealTimeStatsUpdater that does nothing.



-- 
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-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org