Fixup continuation lists for concurrent scavenge backout case #19562
Conversation
|
|
||
| if (NULL != forwardPtr) { | ||
| Assert_MM_false(_markingScheme->isMarked(object)); | ||
| if (!forwardHeader.isSelfForwardedPointer()) { |
There was a problem hiding this comment.
invert the logic (don't use '!') and swap the order
| @@ -329,6 +330,22 @@ MM_MarkingSchemeRootClearer::scanContinuationObjects(MM_EnvironmentBase *env) | |||
| while (NULL != object) { | |||
| gcEnv->_markJavaStats._continuationCandidates += 1; | |||
| omrobjectptr_t next = _extensions->accessBarrier->getContinuationLink(object); | |||
|
|
|||
| { | |||
| /* fixup leftover from backout concurrent scavenge */ | |||
There was a problem hiding this comment.
be more specific.... for example: in case of CS backout make sure the list contains the new version of the objects or if there is only one version, due to being self-forwarded, restore the self-forwarded bit
we could even check that CS backout was on before proceeding with this step, but probably not much savings anyway
| } | ||
| regionExtension->_continuationObjectLists[i].backoutList(); |
There was a problem hiding this comment.
i think this would be easier to read if
- CS check is repeated only once, at the highest possible point (still within while loop), while admittedly at the cost of repeating backout call for evac region
while
if CS
if evac region then backout
else (if not CS)
same as we used to do before the changes
- do 'if evac region' based on lowAddress, rather than the first element of the list
LinHu2016
force-pushed
the
backout_concurrentscavenge
branch
2 times, most recently
from
4e45d96
to
237b446
Compare
| @@ -329,6 +330,26 @@ MM_MarkingSchemeRootClearer::scanContinuationObjects(MM_EnvironmentBase *env) | |||
| while (NULL != object) { | |||
| gcEnv->_markJavaStats._continuationCandidates += 1; | |||
| omrobjectptr_t next = _extensions->accessBarrier->getContinuationLink(object); | |||
|
|
|||
There was a problem hiding this comment.
We should put this block within
#if defined(OMR_GC_CONCURRENT_SCAVENGER)
We do it for other CS aware code in Marking (for example in OMR's MM_MarkingScheme::fixupForwardedSlot)
Still thinking if we should add any runtime check... @dmitripivkine, an opinion?
| regionExtension->_continuationObjectLists[i].backoutList(); | ||
| } | ||
| } | ||
| } else { | ||
| for (uintptr_t i = 0; i < regionExtension->_maxListIndex; i++) { |
There was a problem hiding this comment.
we could pull out even the 2 for loops in front of 'if CS' check'
it's a tradeoff between more code sharing vs performance (check CS for each sublist)
I slightly prefer simpler code, but again asking @dmitripivkine for an opinion
also, please convert the comments to C style /* */
There was a problem hiding this comment.
For this case my vote is for more code sharing indeed. May be caching _extensions->isConcurrentScavengerEnabled() locally in the const bool variable can improve performance (but good compiler should do it already).
There was a problem hiding this comment.
@LinHu2016 sorry for all the confusion
more sharing would mean to push CS check more inside the loops (even inside the for loops)
but still keeping CS if check in front of other if checks for easier understanding
it is not optimal from perf point (to repeat same checks for same regions, while iterating through its sublists), but this is anyway very cheap processing, so we can live with it for the sake of code sharing and clarity
while regions
for region lists
if CS
if evac region then backout list
else (if not CS)
same as we used to do before the changes
There was a problem hiding this comment.
no need to cache isCS enabled flag. as Dmitri said it's probably optimized out, and even if it's not, again, this processing in total is very short, one way or another
LinHu2016
force-pushed
the
backout_concurrentscavenge
branch
2 times, most recently
from
6c41e94
to
bbf14b0
Compare
| /** | ||
| * In case of CS backout make sure the list contains the new version of the objects or if there is only one version, | ||
| * due to being self-forwarded, restore the self-forwarded bit. | ||
| * we could even check that CS backout was on before proceeding with this step, but probably not much savings anyway. |
There was a problem hiding this comment.
actually, let's remove this line of the comment and put the runtime checks instead (I believe at this point backout flag has not been cleared yet)
_extensions->isConcurrentScavengerEnabled() && _extensions->isScavengerBackOutFlagRaised()
| @@ -66,6 +66,8 @@ MM_ScavengerRootScanner::startContinuationProcessing(MM_EnvironmentBase *env) | |||
| if (!list->wasEmpty()) { | |||
| _scavengerDelegate->setShouldScavengeContinuationObjects(true); | |||
| } | |||
| } else { | |||
| list->backupList(); | |||
There was a problem hiding this comment.
While touching the file, fix a few coding standard problems like
- space between if/while and '('
- UDATA -> uintptr_t
*next to variable, rather next to the type
the Objects, which have been copied to survivor or tenure during concurrent scavenge, would not roll back for back out case, so in the following global marking, we need to update the forwardObject in the continuation list. Signed-off-by: hulin <linhu@ca.ibm.com>
LinHu2016
force-pushed
the
backout_concurrentscavenge
branch
from
bbf14b0
to
d70c5d9
Compare
|
jenkins test sanity win,aix jdk21 |
the Objects, which have been copied to survivor or tenure during concurrent scavenge, would not roll back for back out case, so in the following global marking, we need to update the forwardObject in the continuation list.