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.

Sign up for GitHub

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

Jump to bottom

Avoid to scan the finished Continuation Object #16454

Merged
merged 1 commit into from
Jan 12, 2023
Merged

Avoid to scan the finished Continuation Object #16454

merged 1 commit into from
Jan 12, 2023

Conversation

LinHu2016
Copy link
Contributor

There is no need to scan any continuation Objects, which are marked finished, plus there could be potential race condition between continuation concurrent scan and continuation free up for finished Continuation Object.

fix:#15939 (comment)
Signed-off-by: Lin Hu linhu@ca.ibm.com

@LinHu2016
Copy link
Contributor Author

@amicic please review the change, Thanks

amicic
amicic reviewed Dec 12, 2022
View reviewed changes
runtime/oti/VMHelpers.hpp Outdated
@@ -2151,8 +2152,9 @@ class VM_VMHelpers
* For fully STW GCs, there is no harm to scan them, but it's a waste of time since they are scanned during root scanning already.
*
* We don't scan currently scanned either - one scan is enough.
* we don't/no need scan the continuation object before started and after finished.
Copy link
Contributor

Choose a reason for hiding this comment

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

it's clear what we do but not why, add: - java stack does not exist

@amicic
Copy link
Contributor

amicic commented Dec 12, 2022

Jenkins test sanity win jdk19

@dmitripivkine dmitripivkine added comp:vm comp:gc backport:candidate Possible candidate for a backport to a `0.x.y+1` release project:loom Used to track Project Loom related work labels Dec 12, 2022
amicic
amicic reviewed Dec 12, 2022
View reviewed changes
runtime/oti/VMHelpers.hpp Outdated
needScan = started && (NULL != continuation) && (!isContinuationMountedOrConcurrentlyScanned(continuation));
if ((NULL != continuation) && (!isContinuationMountedOrConcurrentlyScanned(continuation))) {
needScan = !J9VMJDKINTERNALVMCONTINUATION_FINISHED(vmThread, continuationObject);
}
Copy link
Contributor

@amicic amicic Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

what was wrong with the previous version of this check?

needScan = started && !finished && (NULL != continuation) && (!isContinuationMountedOrConcurrentlyScanned(continuation));

Copy link
Contributor

@amicic amicic Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

btw, I suspect this is not quite correct order, since setting started flag should be the last thing.
as it's now, it opens a possibility that a third party prematurely starts consuming stack by seeing started flag being set

openj9/runtime/vm/ContinuationHelpers.cpp

Line 168 in 39bceb2

*--currentThread->sp = (UDATA)continuationObject;

@LinHu2016 LinHu2016 force-pushed the GC_update_3 branch 2 times, most recently from e34ac46 to 6dfd732 Compare December 13, 2022 02:39
@amicic amicic self-requested a review December 13, 2022 15:01
@LinHu2016 LinHu2016 force-pushed the GC_update_3 branch 5 times, most recently from acf5e75 to d5d7547 Compare December 16, 2022 16:25
amicic
amicic reviewed Dec 16, 2022
View reviewed changes
runtime/gc_base/GCExtensions.cpp Outdated
*/
if (started && !finished) {
Assert_MM_true(NULL != continuation);
needScan = !isContinuationMountedOrConcurrentlyScanned(continuation);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this won't compile (needs VM_Helpers:: scope, but then it begs a question should more helpers move, as well.
Another dilemma is where we put all of this. For APIs that are used only by GC, this file/class might be slightly better than generic VMHelpers, but possible even better is ContinuationHelpers?

@LinHu2016 LinHu2016 force-pushed the GC_update_3 branch 2 times, most recently from 1bd709e to fa3cb1e Compare December 16, 2022 17:25
@amicic
Copy link
Contributor

amicic commented Dec 16, 2022

jenkins test sanity aix jdk19

amicic
amicic reviewed Jan 11, 2023
View reviewed changes
runtime/gc_base/GCExtensions.cpp
@@ -311,3 +312,30 @@ MM_GCExtensions::releaseNativesForContinuationObject(MM_EnvironmentBase* env, j9
}
#endif /* JAVA_SPEC_VERSION >= 19 */
}

bool
MM_GCExtensions::needScanStacksForContinuationObject(J9VMThread *vmThread, j9object_t objectPtr)
Copy link
Contributor

@amicic amicic Jan 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

try to make this static
(and releaseNativesForContinuationObject as well - not possible, because it uses member continuationListOption)

@amicic
Copy link
Contributor

amicic commented Jan 11, 2023

copyright check (year is 2023 now)

@LinHu2016
Avoid to scan the finished Continuation Object
a9e4bc9 
There is no need to scan any continuation Objects, which are marked
finished, plus there could be potential race condition between
continuation concurrent scan and continuation free up for finished
Continuation Object.

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

amicic commented Jan 12, 2023

jenkins test sanity win jdk19

@amicic amicic merged commit 64f75bd into eclipse-openj9:master Jan 12, 2023
@pshipton pshipton removed the backport:candidate Possible candidate for a backport to a `0.x.y+1` release label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amicic amicic amicic approved these changes

Assignees
No one assigned
Labels
comp:gc comp:vm project:loom Used to track Project Loom related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LinHu2016 @amicic @pshipton @dmitripivkine