[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r494449040
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitorTest.java
##
@@ -33,16 +45,94 @@
@Before
public void setUp() {
-gcStatsMonitor = new GCStatsMonitor(testName.getMethodName());
+ValueMonitor valueMonitor = mock(ValueMonitor.class);
+gcStatsMonitor = new GCStatsMonitor(testName.getMethodName(),
valueMonitor);
+
assertThat(gcStatsMonitor).isNotNull();
-assertThat(gcStatsMonitor.getStatistic("collections")).isEqualTo(0L);
-assertThat(gcStatsMonitor.getStatistic("collectionTime")).isEqualTo(0L);
+assertThat(gcStatsMonitor.getCollections()).isEqualTo(0L);
+assertThat(gcStatsMonitor.getCollectionTime()).isEqualTo(0L);
}
@Test
public void getStatisticShouldReturnZeroForUnknownStatistics() {
assertThat(gcStatsMonitor.getStatistic("unknownStatistic")).isEqualTo(0);
}
+ @Test
+ public void addStatsToMonitor() throws Exception {
+Statistics stats = mock(Statistics.class);
+when(stats.getUniqueId()).thenReturn(11L);
+StatisticDescriptor d1 = mock(StatisticDescriptor.class);
+when(d1.getName()).thenReturn(StatsKey.VM_GC_STATS_COLLECTIONS);
+StatisticDescriptor d2 = mock(StatisticDescriptor.class);
+when(d2.getName()).thenReturn(StatsKey.VM_GC_STATS_COLLECTION_TIME);
+StatisticDescriptor[] descriptors = {d1, d2};
+StatisticsType type = mock(StatisticsType.class);
+when(stats.getType()).thenReturn(type);
+when(type.getStatistics()).thenReturn(descriptors);
+
+when(stats.get(any(StatisticDescriptor.class))).thenReturn(8L, 300L);
Review comment:
This way of specifying return values unnecessarily couples the test to
the current implementation—it insists that the implementation must call
`stats.get(sd)` this many times, and in this specific order.
On this line and the two later ones, it would be better to associate each
value with the appropriate statistic descriptor, the way the other test does.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r494449040
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitorTest.java
##
@@ -33,16 +45,94 @@
@Before
public void setUp() {
-gcStatsMonitor = new GCStatsMonitor(testName.getMethodName());
+ValueMonitor valueMonitor = mock(ValueMonitor.class);
+gcStatsMonitor = new GCStatsMonitor(testName.getMethodName(),
valueMonitor);
+
assertThat(gcStatsMonitor).isNotNull();
-assertThat(gcStatsMonitor.getStatistic("collections")).isEqualTo(0L);
-assertThat(gcStatsMonitor.getStatistic("collectionTime")).isEqualTo(0L);
+assertThat(gcStatsMonitor.getCollections()).isEqualTo(0L);
+assertThat(gcStatsMonitor.getCollectionTime()).isEqualTo(0L);
}
@Test
public void getStatisticShouldReturnZeroForUnknownStatistics() {
assertThat(gcStatsMonitor.getStatistic("unknownStatistic")).isEqualTo(0);
}
+ @Test
+ public void addStatsToMonitor() throws Exception {
+Statistics stats = mock(Statistics.class);
+when(stats.getUniqueId()).thenReturn(11L);
+StatisticDescriptor d1 = mock(StatisticDescriptor.class);
+when(d1.getName()).thenReturn(StatsKey.VM_GC_STATS_COLLECTIONS);
+StatisticDescriptor d2 = mock(StatisticDescriptor.class);
+when(d2.getName()).thenReturn(StatsKey.VM_GC_STATS_COLLECTION_TIME);
+StatisticDescriptor[] descriptors = {d1, d2};
+StatisticsType type = mock(StatisticsType.class);
+when(stats.getType()).thenReturn(type);
+when(type.getStatistics()).thenReturn(descriptors);
+
+when(stats.get(any(StatisticDescriptor.class))).thenReturn(8L, 300L);
Review comment:
This way of specifying return values unnecessarily couples the test to
the current implementation—it insists that the implementation must call
`stats.get(sd)` this many times, and in this specific order.
On this line and the two later ones, it would be better to associate each
value with the appropriate statistic descriptor, the way the other test does.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493890530
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
Review comment:
Those are currently the only two descriptors in the GC statistics type,
and the only two values reported by GarbageCollectorMXBean (the ultimate source
of the values). If another descriptor were to be added to the statistics type
in the future, we'd want to review it before deciding how `GCStatsMonitor`
should handle it. So I think it's appropriate to ignore any descriptors we
don't recognize.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493853151
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
Let me correct my terminology a bit… I keep saying "GC type," and that's
not the right distinction. What distinguishes one batch of GC stats from
another is which GarbageCollectorMXBean the values come from. I think (but I'm
not sure) that each GC bean reports about a particular memory space.
So where I say "GC type," a better term might be "memory space".
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493844666
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
> then what would those count/time mean though. It's not right to report
just a single one of them. It's not right to sum them up either then
The stats notification stuff is specifically designed to report only
changes. As a result, old gen and new gen CGs are almost always reported
independently (unless they both happen to change during a single sample).
I think that's why the old code was doing the weird stuff. It was trying to
track the previous value for each kind of GC, so that when a new value arrived,
it could add in just the change in that kind of GC. Decrementing and
incrementing was a puzzling way to do that, but that was the goal.
Here's a way that seems simpler to me than decrementing/incrementing. I'll
use collections as an example, but it's the same for collectionTime.
- Create a map that records the latest value for each GC type.
- Create a field that records the sum of values for all types. This is what
gets reported via getCollections()
- When a new kind of GC is added via addStatisticsToMonitor()…
1. Store the initial value for that GC type in the map.
2. Sum the values of all GC types in the map, and store the sum in the
field.
- When a notification arrives…
1. Store the new value for that GC type in the map.
2. Sum the values of all GC types in the map, and store the sum in the
field.
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493761722
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
I think a notification includes only those stats whose values have
changed. If that's correct, what we're summing here is only the _changed_ GC
stats, not _all_ of the GC stats. This will work only if we're absolutely
certain that every stat will change on every sample.
See `ValueMonitor.monitorStatistics()`, which builds the notification only
from updated stats.
See `SampleCollector.sample()`, which adds a stats to the updated stats list
only if the value of the stat changed from the previous sample.
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
