Skip to content

Modify scavenger GC count #7814

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

Merged
merged 1 commit into from
Jul 7, 2025
Merged

Conversation

adpopescu
Copy link
Contributor

@adpopescu adpopescu commented Jun 26, 2025

Currently, scavengerStats is used to count increments and incrementScavengerStats is not used to count anything. This change aligns scavengerStats with cycles and incrementScavengerStats with increments.

@adpopescu
Copy link
Contributor Author

@amicic

@amicic
Copy link
Contributor

amicic commented Jun 27, 2025

this can be simplified away, so that we have only the else clause

bool
MM_ScavengerStats::isAvailable(MM_EnvironmentBase *env) {
	if (env->getExtensions()->isConcurrentScavengerEnabled()) {
		/* with CS, we count STW increments (2 per each cycle) */
		return (_gcCount > 1);
	} else {
		return (_gcCount > 0);
	}
}

@adpopescu
Copy link
Contributor Author

I've made the change to isAvailable as well @amicic

@amicic
Copy link
Contributor

amicic commented Jul 3, 2025

I've made the change to isAvailable as well @amicic

Thanks. While this is a good simplification, looking more deeply we may have a problem....

MM_ScavengerStats::isAvailable() is used by Concurrent Global GC to determine that ScavengerStats have been gathered, which means that a full cycle has to complete.

So, the original spot where we moved gcCount++ is too early. There is a spot later in MM_Scavenger::mainThreadGarbageCollect that checks if it's the last increment that should be better. Let's put it first thing in that block.

@adpopescu
Copy link
Contributor Author

I moved it to after it reports the cycle end.

@adpopescu adpopescu force-pushed the jfr-gc-events branch 2 times, most recently from 3c9ed32 to a145b26 Compare July 3, 2025 17:46
@amicic
Copy link
Contributor

amicic commented Jul 3, 2025

Let's try to piggyback one more thing to this:

let's introduce a clone of j9gc_get_unique_GC_count, but have it based on OMR/GC (nothing J9 specific):

GCExtensions::getUniqueGCCycleCount()

which will then be called by j9gc_get_unique_GC_count (updated later in a J9 PR) and potentially used elsewhere in OMR/GC

@amicic amicic closed this Jul 3, 2025
@amicic
Copy link
Contributor

amicic commented Jul 3, 2025

argh, reopening

@amicic amicic reopened this Jul 3, 2025
@adpopescu
Copy link
Contributor Author

I've added the get function as well.

@amicic
Copy link
Contributor

amicic commented Jul 3, 2025

I've added the get function as well.

there is a spot you originally updated (but reverted) in Collector.cpp where you can use it

@amicic
Copy link
Contributor

amicic commented Jul 3, 2025

@dsouzai please, reivew/merge

@amicic
Copy link
Contributor

amicic commented Jul 4, 2025

jenkins build all

@amicic
Copy link
Contributor

amicic commented Jul 4, 2025

@babsingh please, review/merge

@dsouzai
Copy link
Contributor

dsouzai commented Jul 4, 2025

I'm ok to step aside to let Babneet be committer for this PR since it involves changes outside the compiler; however, this PR is still in draft state, so it will need to be marked Ready for review before it can be merged.

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

minor nits

@adpopescu adpopescu marked this pull request as ready for review July 4, 2025 16:53
@adpopescu adpopescu requested a review from babsingh July 4, 2025 19:29
Currently, scavengerStats is used to count increments and incrementScavengerStats is not used to count anything. This change aligns scavengerStats with cycles and incrementScavengerStats with increments.

Signed-off-by: Adrian Popescu <[email protected]>
@babsingh
Copy link
Contributor

babsingh commented Jul 7, 2025

jenkins build all

@babsingh babsingh merged commit 44e26ad into eclipse-omr:master Jul 7, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants