Fix issue relate with CollectionUsageThreshold of memory pool #2993

LinHu2016 · 2018-09-24T15:38:46Z

we should only check if the notification for
CollectionUsageThreshold need to be fired at the end of the collection,
which try to recycle the memory pool.
for BackCompatible mode scavenge does not try to reclaim memory from
the whole heap, so we do not fire the notification of
CollectionUsageThreshold at the end scavenge.
record the last GC ID for retrieving more detail gcInfo of last
GC from J9GarbageCollectorData
MemoryPoolMXBean.getCollectionUsage() getPreCollectionUsage()

and isCollectionUsageThresholdExceeded() also match the same rule.
fix: #2982

dmitripivkine · 2018-09-24T16:16:48Z

runtime/jcl/common/mgmtinit.c

-		/* if a heap collection threshold is set, check whether we are above or below */
-		if (0 < memoryPool->collectionUsageThreshold) {
+		/* if a memory pool collection threshold is set and the memory pool is manaaged by the collector, check whether we are above or below */
+		if (0 < memoryPool->collectionUsageThreshold && j9gc_is_managedpool_by_collector(vm, (UDATA)(gcID & J9VM_MANAGEMENT_GC_HEAP_ID_MASK), (UDATA)(memoryPool->id & J9VM_MANAGEMENT_POOL_HEAP_ID_MASK))) {


Would you please add extra brackets around "0 < memoryPool->collectionUsageThreshold" ?

dmitripivkine · 2018-09-24T16:22:08Z

There is a similar pattern in Java_com_ibm_java_lang_management_internal_MemoryPoolMXBeanImpl_isCollectionUsageThresholdExceededImpl(). Should it be updated as well?

dmitripivkine · 2018-09-24T17:30:47Z

runtime/jcl/common/mgmtinit.c

@@ -632,8 +632,8 @@ gcEndEvent(J9JavaVM *vm, UDATA heapSize, UDATA heapUsed, UDATA *totals, UDATA *f
 			memoryPool->peakMax = memoryPool->postCollectionMaxSize;
 		}

-		/* if a heap collection threshold is set, check whether we are above or below */
-		if (0 < memoryPool->collectionUsageThreshold) {
+		/* if a memory pool collection threshold is set and the memory pool is manaaged by the collector, check whether we are above or below */


typo manaaged -> managed

LinHu2016 · 2018-09-27T17:52:10Z

update for MemoryPoolMXBean.getCollectionUsage() getPreCollectionUsage() and isCollectionUsageThresholdExceeded() as well

dmitripivkine · 2018-09-27T18:29:51Z

runtime/jcl/common/mgmtinit.c

@@ -546,7 +552,8 @@ gcStartEvent(J9JavaVM *vm, UDATA heapSize, UDATA heapUsed, UDATA *totals, UDATA
 							notification->usageThreshold->poolID = memoryPool->id;
 							notification->usageThreshold->usedSize = used;
 							notification->usageThreshold->totalSize = total;
-							notification->usageThreshold->maxSize = memoryPool->preCollectionMaxSize;
+//							notification->usageThreshold->maxSize = memoryPool->preCollectionMaxSize;


I guess this line should be removed

dmitripivkine · 2018-09-27T18:32:08Z

runtime/jcl/common/mgmtmempool.c

+{
+	UDATA idx = 0;
+
+	for(; idx < mgmt->supportedCollectors; ++idx) {


Need space between "for" and bracket. Also I would provide initial value explicitly:
for (idx=0; idx < mgmt->supportedCollectors; ++idx) {

dmitripivkine

LGTM

DanHeidinga · 2018-10-12T13:39:26Z

@dmitripivkine @LinHu2016 Is this required for the 0.11.0 release? How risky is the change?

dmitripivkine · 2018-10-12T13:59:38Z

@dmitripivkine @LinHu2016 Is this required for the 0.11.0 release? How risky is the change?

I believe the risk is medium but limited to MemoryPoolMXBean functionality. @LinHu2016 is there urgency for this?

DanHeidinga

one minor nitpick which isn't strictly required and the code looks ok to me

DanHeidinga · 2018-10-12T14:32:25Z

runtime/gc_base/modronapi.cpp

+		if (J9_GC_MANAGEMENT_COLLECTOR_SCAVENGE == gcID) {
+			/* for BackCompatible mode scavenge does not try to reclaim memory from the whole heap, so we do not mark JavaHeap managed by scavenge */
+			return 0;
+		} else {


The else block is unnecessary as the if block returns. Can be written as:

if (J9_GC_MANAGEMENT_COLLECTOR_SCAVENGE == gcID) { /* for BackCompatible mode scavenge does not try to reclaim memory from the whole heap, so we do not mark JavaHeap managed by scavenge */ return 0; } return 1;

LinHu2016 · 2018-10-12T15:28:30Z

@DanHeidinga @dmitripivkine As Dmitri said without the fix, it would be limited to MemoryPoolMXBean CollectionUsage related functionality, but it is not urgency.

DanHeidinga · 2018-10-12T15:41:23Z

Jenkins test sanity,functional zlinux,win jdk8

DanHeidinga · 2018-10-12T15:42:04Z

Jenkins test sanity,functional zlinux jdk8

DanHeidinga · 2018-10-12T15:42:21Z

Jenkins test sanity zlinux jdk8

DanHeidinga · 2018-10-12T15:42:38Z

Jenkins test extended zlinux jdk8

DanHeidinga · 2018-10-12T15:43:08Z

Jenkins test sanity win jdk10

DanHeidinga · 2018-10-12T16:57:47Z

Windows compile failure:

12:05:28 ../common/mgmtinit.c(623) : error C2220: warning treated as error - no 'object' file generated
12:05:28 ../common/mgmtinit.c(623) : warning C4244: '=' : conversion from 'UDATA' to 'U_32', possible loss of data
12:05:28 mgmtos.c

DanHeidinga · 2018-10-12T17:52:22Z

@LinHu2016 are you looking at the compile failure?

- we should only check if the notification for CollectionUsageThreshold need to be fired at the end of the collection, which try to recycle the memory pool. - for BackCompatible mode scavenge does not try to reclaim memory from the whole heap, so we do not fire the notification of CollectionUsageThreshold at the end scavenge. - record the last GC ID for retrieving more detail gcInfo of last GC from J9GarbageCollectorData - MemoryPoolMXBean.getCollectionUsage() getPreCollectionUsage() and isCollectionUsageThresholdExceeded() also match the same rule. Signed-off-by: Lin Hu <linhu@ca.ibm.com>

LinHu2016 · 2018-10-15T13:53:46Z

fixed the compiling error.

DanHeidinga · 2018-10-16T02:51:09Z

Jenkins test all zlinux jdk8

DanHeidinga · 2018-10-16T02:51:25Z

Jenkins test sanity win jdk10

pshipton · 2018-10-16T19:20:12Z

Reverted until the problems can be fixed, since the change is causing test failures

j> 14:09:22 Exception in thread "MemoryMXBean notification dispatcher" java.lang.IllegalArgumentException: committed value cannot be larger than the max value
j> 14:09:22 	at java.lang.management.MemoryUsage.<init>(MemoryUsage.java:118)
j> 14:09:22 	at com.ibm.lang.management.internal.ExtendedGarbageCollectorMXBeanImpl.buildGcInfo(ExtendedGarbageCollectorMXBeanImpl.java:94)
j> 14:09:22 	at com.ibm.lang.management.internal.MemoryNotificationThread.dispatchGCNotificationHelper(MemoryNotificationThread.java:95)
j> 14:09:22 	at com.ibm.lang.management.internal.MemoryNotificationThread.processNotificationLoop(Native Method)
j> 14:09:22 	at com.ibm.lang.management.internal.MemoryNotificationThread.run(MemoryNotificationThread.java:183)

LinHu2016 force-pushed the issue2982 branch from aa8bfdb to cbfd7b8 Compare September 24, 2018 15:40

pshipton added the comp:gc label Sep 24, 2018

dmitripivkine reviewed Sep 24, 2018

View reviewed changes

dmitripivkine self-requested a review September 24, 2018 16:17

LinHu2016 force-pushed the issue2982 branch 3 times, most recently from 3d97c85 to 4fdc078 Compare September 24, 2018 16:43

dmitripivkine reviewed Sep 24, 2018

View reviewed changes

LinHu2016 force-pushed the issue2982 branch 9 times, most recently from f6d37d8 to ddf4792 Compare September 27, 2018 14:21

dmitripivkine added the comp:vm label Sep 27, 2018

dmitripivkine requested a review from DanHeidinga September 27, 2018 18:25

dmitripivkine reviewed Sep 27, 2018

View reviewed changes

LinHu2016 force-pushed the issue2982 branch from ddf4792 to 3478041 Compare September 27, 2018 18:45

dmitripivkine approved these changes Sep 27, 2018

View reviewed changes

DanHeidinga reviewed Oct 12, 2018

View reviewed changes

LinHu2016 force-pushed the issue2982 branch from 3478041 to 7ce3efa Compare October 12, 2018 15:33

DanHeidinga self-assigned this Oct 12, 2018

LinHu2016 force-pushed the issue2982 branch from 7ce3efa to 85f9c3b Compare October 15, 2018 13:51

DanHeidinga merged commit 1e9f12d into eclipse-openj9:master Oct 16, 2018

gacholio mentioned this pull request Oct 16, 2018

Revert "Fix issue relate with CollectionUsageThreshold of memory pool" #3308

Merged

LinHu2016 mentioned this pull request Apr 2, 2019

Fix issue relate with CollectionUsageThreshold of memory pool #5324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue relate with CollectionUsageThreshold of memory pool #2993

Fix issue relate with CollectionUsageThreshold of memory pool #2993

LinHu2016 commented Sep 24, 2018 •

edited

Loading

dmitripivkine Sep 24, 2018

LinHu2016 Sep 24, 2018

dmitripivkine commented Sep 24, 2018

dmitripivkine Sep 24, 2018

LinHu2016 commented Sep 27, 2018

dmitripivkine Sep 27, 2018

LinHu2016 Sep 27, 2018

dmitripivkine Sep 27, 2018 •

edited

Loading

LinHu2016 Sep 27, 2018

dmitripivkine left a comment

DanHeidinga commented Oct 12, 2018

dmitripivkine commented Oct 12, 2018

DanHeidinga left a comment

DanHeidinga Oct 12, 2018

LinHu2016 Oct 12, 2018

LinHu2016 commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

LinHu2016 commented Oct 15, 2018

DanHeidinga commented Oct 16, 2018

DanHeidinga commented Oct 16, 2018

pshipton commented Oct 16, 2018 •

edited

Loading

Fix issue relate with CollectionUsageThreshold of memory pool #2993

Fix issue relate with CollectionUsageThreshold of memory pool #2993

Conversation

LinHu2016 commented Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine commented Sep 24, 2018

Choose a reason for hiding this comment

LinHu2016 commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine left a comment

Choose a reason for hiding this comment

DanHeidinga commented Oct 12, 2018

dmitripivkine commented Oct 12, 2018

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinHu2016 commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

DanHeidinga commented Oct 12, 2018

LinHu2016 commented Oct 15, 2018

DanHeidinga commented Oct 16, 2018

DanHeidinga commented Oct 16, 2018

pshipton commented Oct 16, 2018 • edited Loading

LinHu2016 commented Sep 24, 2018 •

edited

Loading

dmitripivkine Sep 27, 2018 •

edited

Loading

pshipton commented Oct 16, 2018 •

edited

Loading