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

Remove flushCachesForGC from acquireExclusiveForGC path #7265

Merged
merged 1 commit into from Feb 15, 2024

Conversation

amicic
Copy link
Contributor

@amicic amicic commented Feb 13, 2024

flushCachesForGC is mostly called close/prior to start-gc point for each particual GC operation/STW-increment, so that we often end up doing it twice, the first one being early during acquireExclusiveVMAccessForGC negotiation.

This change removes the one from acquireExclusiveVMAccessForGC, but also adds a couple of flushCachesForGC in GC specific operation that relied on acquireExclusiveVMAccessForGC to do it.

@amicic amicic changed the title Remove flushCachesForGC from acquireExclusiveForGC path WIP: Remove flushCachesForGC from acquireExclusiveForGC path Feb 13, 2024
@amicic amicic added the comp:gc label Feb 13, 2024
@amicic
Copy link
Contributor Author

amicic commented Feb 13, 2024

@dmitripivkine still testing, but have a look

@amicic amicic force-pushed the flushCachesForGC branch 7 times, most recently from b35f89b to e9f3235 Compare February 14, 2024 17:30
@amicic amicic changed the title WIP: Remove flushCachesForGC from acquireExclusiveForGC path Remove flushCachesForGC from acquireExclusiveForGC path Feb 14, 2024
@amicic
Copy link
Contributor Author

amicic commented Feb 14, 2024

@dmitripivkine Ready for review, but can only be merged after eclipse-openj9/openj9#18950.

flushCachesForGC is mostly called close/prior to start-gc point for each
particual GC operation/STW-increment, so that we often end up doing it
twice, the first one being early during acquireExclusiveVMAccessForGC
negotiation.

This change removes the one from acquireExclusiveVMAccessForGC, but also
adds a couple of flushCachesForGC in GC specific operation that relied
on acquireExclusiveVMAccessForGC to do it.

Signed-off-by: Aleksandar Micic <Aleksandar_Micic@ca.ibm.com>
@dmitripivkine
Copy link
Contributor

@babsingh Would you please to help to merge this item when eclipse-openj9/openj9#18950 is going to be promoted first?

@amicic
Copy link
Contributor Author

amicic commented Feb 14, 2024

Beside some code cleanup/simplification, it also slightly reduces the pause times, in order of 10s of micro seconds (dependent on the number of application threads).
Here is the effect on the first increment of Concurrent Scavenge (1.237ms vs 1.2ms).

grep -A20 '<cycle-start.scavenge' jbb_CS_PRESET9000_base.out |grep '<gc-op'|awk -F" '{x+=$6} END {print x/NR}'
1.23754
grep -A20 '<cycle-start.scavenge' jbb_CS_PRESET9000_noFlushOnExclusive.out |grep '<gc-op'|awk -F" '{x+=$6} END {print x/NR}'
1.19978

@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

@dmitripivkine eclipse-openj9/openj9#18950 has been merged. Are there any other dependencies or is this PR good to be merged?

@amicic
Copy link
Contributor Author

amicic commented Feb 14, 2024

there is no more dependancy

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.

OSX PR build failed due to a known and unrelated issue: #7181.

@babsingh babsingh merged commit 91f005a into eclipse:master Feb 15, 2024
16 of 18 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.

None yet

3 participants