Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue relate with CollectionUsageThreshold of memory pool #5324

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

LinHu2016
Copy link
Contributor

- 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 J9GarbageCollectorDataMemoryPoolMXBean
.getCollectionUsage() getPreCollectionUsage() and
isCollectionUsageThresholdExceeded() also match the same rule.
   - fix potential IllegalArgumentException issue.

relate:#2993

@pshipton
Copy link
Member

pshipton commented Apr 8, 2019

jenkins test sanity plinux jdk8

@pshipton
Copy link
Member

pshipton commented Apr 8, 2019

jenkins test extended plinux jdk8

@pshipton
Copy link
Member

pshipton commented Apr 8, 2019

jenkins compile win jdk8

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having some trouble understanding this change as there's 1 gcInfo and potentially many memoryPools and this change seems to be collecting the data in the gcInfo and then applying it to the memoryPools.

I don't understand why its being changed this way rather than keeping the data in the memory pools and agregating it in the gcInfo.

Once I understand that, I'll be in a better position to review this.

runtime/jcl/common/mgmtinit.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtinit.c Show resolved Hide resolved
@LinHu2016
Copy link
Contributor Author

I'm having some trouble understanding this change as there's 1 gcInfo and potentially many memoryPools and this change seems to be collecting the data in the gcInfo and then applying it to the memoryPools.

I don't understand why its being changed this way rather than keeping the data in the memory pools and agregating it in the gcInfo.

Once I understand that, I'll be in a better position to review this.

Hi @DanHeidinga, according to JAVA Spec, preCollection(IBM Java only), postCollection Memory Usage in memoryPool only keep information for the GC, which recycle the memory pool. For the case gencon, Tenure memorypool would only update after global gc, scavenge does not reclaim tenure memorypool( actually it consumes tenure space).

for the details, please check java doc for MemoryPoolMXBean.getCollectionUsage()
https://docs.oracle.com/javase/7/docs/api/java/lang/management/MemoryPoolMXBean.html#getCollectionUsage()

@LinHu2016 LinHu2016 force-pushed the issue2982 branch 4 times, most recently from 5b4ed5f to d35c047 Compare April 18, 2019 18:05
@pshipton
Copy link
Member

@DanHeidinga I note there are new changes pushed.

@pshipton
Copy link
Member

pshipton commented Jun 3, 2019

@LinHu2016 is this ready for re-review?

@LinHu2016
Copy link
Contributor Author

@pshipton yes, the new update is ready for reviewing.

@pshipton
Copy link
Member

pshipton commented Jun 4, 2019

@DanHeidinga fyi

@pshipton
Copy link
Member

I don't expect this is going to be ready for the 0.15 release.

@pshipton
Copy link
Member

Getting late to put into the 0.17 release, moving forward again.

@pshipton
Copy link
Member

pshipton commented Dec 4, 2019

@DanHeidinga any plans to review this before the 0.18 release, or should it be moved to the next?

@DanHeidinga
Copy link
Member

Will try and get through it this week (and apologies on the delay here)

runtime/gc_base/modronapi.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtinit.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtmempool.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtmempool.c Show resolved Hide resolved
runtime/jcl/common/mgmtmempool.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtmempool.c Outdated Show resolved Hide resolved
runtime/jcl/common/mgmtmempool.c Outdated Show resolved Hide resolved
@DanHeidinga
Copy link
Member

@LinHu2016 Can you rebase this so it only includes your changes?

1 similar comment
@DanHeidinga
Copy link
Member

@LinHu2016 Can you rebase this so it only includes your changes?

@LinHu2016
Copy link
Contributor Author

@LinHu2016 Can you rebase this so it only includes your changes?

the branch has been rebased with the latest master.

	- 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 J9GarbageCollectorDataMemoryPoolMXBean
	.getCollectionUsage() getPreCollectionUsage() and
	isCollectionUsageThresholdExceeded() also match the same rule.

Signed-off-by: Lin Hu <linhu@ca.ibm.com>
@LinHu2016
Copy link
Contributor Author

update copyright to 2020

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk8

@DanHeidinga
Copy link
Member

Jenkins test extended zlinux jdk11

@DanHeidinga DanHeidinga self-assigned this Jan 14, 2020
@DanHeidinga
Copy link
Member

Jenkins test extended zlinux jdk11

@DanHeidinga
Copy link
Member

Relaunched the extended testing as it failed. @LinHu2016 If it fails again, can you rebase to the latest and we'll rerun the builds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants