-
Notifications
You must be signed in to change notification settings - Fork 729
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
Initial changes to refactor snapshot at the beginning barrier. #2298
Conversation
Work in progress. @amicic @dmitripivkine Aleks, Dmitri could you please provide your feedback and comments. Thank you! omr pull request: eclipse-omr/omr#2717 |
@@ -88,6 +88,9 @@ class MM_ConfigurationDelegate | |||
if (extensions->alwaysCallWriteBarrier) { | |||
writeBarrierType = gc_modron_wrtbar_always; | |||
} | |||
if (extensions->isSnapshotAtTheBeginningBarrierEnabled()) { | |||
writeBarrierType = gc_modron_wrtbar_realtime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gc_modron_wrtbar_realtime should change into gc_modron_wrtbar_satb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to barrier to gc_modron_wrtbar_satb. I'm keeping gc_modron_wrtbar_realtime in omr until the initial opnej9 changes get committed.
if (NULL == _extensions->sATBBarrierRememberedSet) { | ||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I have a feeling this should be in OMR part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make MM_RememberedSetWorkPackets to take workPackets as a specific MM_WorkPacketsSATB type, it's safe to upcast markingScheme->getWorkPackets() to (MM_WorkPacketsSATB* ), since it's under isSATBEnabled check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this code to OMR.
* is in progress is to be remembered for later marking and scanning. | ||
*/ | ||
void | ||
MM_StandardAccessBarrier::rememberObject(MM_EnvironmentBase *env, J9Object *object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will now have 2 completely different remember actions in barrierr world:
- as part of a generational barrier, we need to remember an old object (referencing a new object); the action could be more specifically called remember-object-as-root (in subsequent GCs)
- as part SATB concurrent marking, we need to remember old reference point being overwritten; the action could be more specifically called remember-object-to-rescan (in this GC cycle)
Therefore, the name of methods should be appropriately renamed to avoid confusion. Renaming this method should be sufficent, for now. Creative suggestions are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed the method to rememberObjectToRescan
} | ||
MM_EnvironmentBase* env = MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread); | ||
|
||
if (isBarrierActive(env)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two isBarrierActive method names should be more specific (refer to SATB)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed the helper to isSATBBarrierActive.
MM_StandardAccessBarrier::preObjectStoreInternal(J9VMThread *vmThread, J9Object *destObject, fj9object_t *destAddress, J9Object *value, bool isVolatile) | ||
{ | ||
J9JavaVM *javaVM = vmThread->javaVM; | ||
if (javaVM->gcWriteBarrierType != gc_modron_wrtbar_realtime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, but this check should be part of isBarrierActive(). the point is minimize the amount of barrier type (in this case SATB) specific code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the check to isSATBBarrierActive helper.
J9JavaVM *javaVM = vmThread->javaVM; | ||
if (javaVM->gcWriteBarrierType == gc_modron_wrtbar_realtime) { | ||
return retValue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to other reviewers: I'm ok with this hack for now (it forces slow path array copying).
25f847a
to
7249204
Compare
@@ -1106,7 +1106,7 @@ J9::Options::fePreProcess(void * base) | |||
case j9gc_modron_wrtbar_cardmark: wrtbarMode = TR_WrtbarCardMark; break; | |||
case j9gc_modron_wrtbar_cardmark_and_oldcheck: wrtbarMode = TR_WrtbarCardMarkAndOldCheck; break; | |||
case j9gc_modron_wrtbar_cardmark_incremental: wrtbarMode = TR_WrtbarCardMarkIncremental; break; | |||
case j9gc_modron_wrtbar_realtime: wrtbarMode = TR_WrtbarRealTime; break; | |||
case j9gc_modron_wrtbar_satb: wrtbarMode = TR_WrtbarRealTime; break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have j9gc_modron_wrtbar_satb_and_oldcheck defined and checked at various spots where j9gc_modron_wrtbar_satb is checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
MM_StandardAccessBarrier::rememberObjectToRescan(MM_EnvironmentBase *env, J9Object *object) | ||
{ | ||
//TODO SATB re-enable opt? | ||
//if (_markingScheme->markObject(env, object, true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be in. otherwise, it will create way too many unnecessary/duplicate remembered objects (and overstress marking work stack)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -481,6 +604,10 @@ I_32 | |||
MM_StandardAccessBarrier::backwardReferenceArrayCopyIndex(J9VMThread *vmThread, J9IndexableObject *srcObject, J9IndexableObject *destObject, I_32 srcIndex, I_32 destIndex, I_32 lengthInSlots) | |||
{ | |||
I_32 retValue = ARRAY_COPY_NOT_DONE; | |||
//TODO SATB re-enable opt? | |||
if (_extensions->configuration->isSnapshotAtTheBeginningBarrierEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this (opt for ref array copying) is ok to disable, for the initial contribution
runtime/oti/j9accessbarrier.h
Outdated
@@ -171,13 +171,13 @@ typedef struct J9IndexableObject* mm_j9array_t; | |||
(void)0) | |||
#define J9OBJECT__PRE_OBJECT_STORE_ADDRESS(vmThread, object, address, value) \ | |||
((void)0, \ | |||
(OMR_GC_ALLOCATION_TYPE_SEGREGATED == (J9VMTHREAD_JAVAVM((vmThread)))->gcAllocationType) ? \ | |||
(OMR_GC_WRITE_BARRIER_TYPE_REALTIME == (J9VMTHREAD_JAVAVM((vmThread)))->gcWriteBarrierType) ? \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should check for OMR_GC_WRITE_BARRIER_TYPE_SATB || OMR_GC_WRITE_BARRIER_TYPE_SATB_AND_OLDCHECK, but I guess you did not introduce them yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did introduce them in OMR but I missed this spot, thank you for catching it. I will update the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
MMINLINE bool isSATBBarrierActive(MM_EnvironmentBase* env) | ||
{ | ||
return _extensions->configuration->isSnapshotAtTheBeginningBarrierEnabled() && | ||
!_extensions->sATBBarrierRememberedSet->isGlobalFragmentIndexPreserved(env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first part might not be necessary, since the second part would never be satisfied if SATB is not enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
if (_extensions->configuration->isSnapshotAtTheBeginningBarrierEnabled()) { | ||
|
||
_extensions->sATBBarrierRememberedSet->initializeFragment(env, &(((J9VMThread *)env->getLanguageVMThread())->sATBBarrierRememberedSetFragment)); | ||
//TODO: SATB re-enable later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep this in, but we'll worry about double barrier in more details later.
for example, we don't have yet the mechanism to enable/disable double barrier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncommented it.
/** | ||
* Called after an object is stored into another object. | ||
*/ | ||
void | ||
MM_StandardAccessBarrier::postObjectStore(J9VMThread *vmThread, J9Object *destObject, fj9object_t *destAddress, J9Object *value, bool isVolatile) | ||
{ | ||
postObjectStoreImpl(vmThread, destObject, value); | ||
if (_extensions->configuration->isIncrementalUpdateBarrierEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly to isSATBBarrierActive, there should be a helper isIncrementalUpdateBarrierActive within this class.
then, that helper probably does not need to go as far as to configuration, but just check in thread local data if the barrier is active. it should not ever be really active on thread level, if in configuration it's not enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isIncrementalUpdateBarrierEnabled checks for barrier type which I can only get from configuration at this point. Since I don't want to execute postObjectStoreImpl for SATB barriers. I think we talked that eventually we might move the generational barrier code to preObjectStoreImpl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm realizing it's a bit more complex because we also have generational barrier. If we decide to leave it as a post barrier , than we should check if either of Incremental update is active or generational barrier is enabled.
I believe that generational barrier does not have to be post barrier. So we could have it either pre or post, depending if we run SATB or IncrementalUpdate for Concurrent Mark.
It's probably safest not to change runtime behaviour in existing configuration (and leave it as a post), although it might mean not the simplest/optimal code.
Lastly, do we really need this explict check here? We already have vmThread->privateFlags & J9_PRIVATE_FLAGS_CONCURRENT_MARK_ACTIVE check inside postObjectStoreImpl, which should turn into something that is IncurementalUpdate specific, if already not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now moved it to a helper with isIncrementalUpdateBarrierEnabled check.
@@ -92,6 +99,28 @@ class MM_StandardAccessBarrier : public MM_ObjectAccessBarrier | |||
virtual bool preMonitorTableSlotRead(J9JavaVM *vm, j9object_t *srcAddress); | |||
#endif | |||
|
|||
/* New methods */ | |||
bool preObjectStoreInternal(J9VMThread *vmThread, J9Object *destObject, fj9object_t *destAddress, J9Object *value, bool isVolatile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align the naming... there is already postObjectStoreImpl, so this should be probably preObjectStoreImpl? although throughout GC code we tend more often to use ...Internal than ...Impl. do a quick check what is more common before changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed methods to preObjectStoreImpl since all other methods in this file follow Impl convention.
@@ -2625,7 +2625,8 @@ class MM_ObjectAccessBarrierAPI | |||
VMINLINE void | |||
internalPreStoreObject(J9VMThread *vmThread, j9object_t object, fj9object_t *destAddress, j9object_t value) | |||
{ | |||
if (j9gc_modron_wrtbar_realtime == _writeBarrierType) { | |||
if ((j9gc_modron_wrtbar_satb == _writeBarrierType) || | |||
(j9gc_modron_wrtbar_satb_and_oldcheck == _writeBarrierType)) { | |||
internalPreStoreObjectRealtime(vmThread, object, destAddress, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename internalPreStoreObjectRealtime to internalPreStoreObjectSATB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it's not, you did it just for static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, renamed both now
@@ -2644,7 +2645,8 @@ class MM_ObjectAccessBarrierAPI | |||
VMINLINE void | |||
internalStaticPreStoreObject(J9VMThread *vmThread, j9object_t object, j9object_t *destAddress, j9object_t value) | |||
{ | |||
if (j9gc_modron_wrtbar_realtime == _writeBarrierType) { | |||
if (j9gc_modron_wrtbar_satb == _writeBarrierType || | |||
j9gc_modron_wrtbar_satb_and_oldcheck == _writeBarrierType) { | |||
internalStaticPreStoreObjectRealtime(vmThread, object, destAddress, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to SATB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
runtime/oti/j9accessbarrier.h
Outdated
@@ -171,13 +171,13 @@ typedef struct J9IndexableObject* mm_j9array_t; | |||
(void)0) | |||
#define J9OBJECT__PRE_OBJECT_STORE_ADDRESS(vmThread, object, address, value) \ | |||
((void)0, \ | |||
(OMR_GC_ALLOCATION_TYPE_SEGREGATED == (J9VMTHREAD_JAVAVM((vmThread)))->gcAllocationType) ? \ | |||
((OMR_GC_WRITE_BARRIER_TYPE_SATB || OMR_GC_WRITE_BARRIER_TYPE_SATB_AND_OLDCHECK) == (J9VMTHREAD_JAVAVM((vmThread)))->gcWriteBarrierType) ? \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually not correct. the 'or' condition should be between two equality comparisons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
2ff130e
to
be8ddc6
Compare
runtime/oti/j9accessbarrier.h
Outdated
J9VMTHREAD_JAVAVM((vmThread))->memoryManagerFunctions->J9MetronomeWriteBarrierStore((vmThread), (j9object_t)(object), (fj9object_t*)(address), (j9object_t)(value)) : \ | ||
(void)0 \ | ||
) | ||
#define J9OBJECT__PRE_OBJECT_STORE_ADDRESS_VM(javaVM, object, address, value) \ | ||
((void)0, \ | ||
(OMR_GC_ALLOCATION_TYPE_SEGREGATED == javaVM->gcAllocationType) ? \ | ||
((OMR_GC_WRITE_BARRIER_TYPE_SATB == (J9VMTHREAD_JAVAVM((vmThread)))->gcWriteBarrierType) || \ | ||
(OMR_GC_WRITE_BARRIER_TYPE_SATB_AND_OLDCHECK == (J9VMTHREAD_JAVAVM((vmThread)))->gcWriteBarrierType)) ? \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this API provides javaVM, not vmThread
@@ -2644,8 +2645,9 @@ class MM_ObjectAccessBarrierAPI | |||
VMINLINE void | |||
internalStaticPreStoreObject(J9VMThread *vmThread, j9object_t object, j9object_t *destAddress, j9object_t value) | |||
{ | |||
if (j9gc_modron_wrtbar_realtime == _writeBarrierType) { | |||
internalStaticPreStoreObjectRealtime(vmThread, object, destAddress, value); | |||
if (j9gc_modron_wrtbar_satb == _writeBarrierType || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add extra brackets
runtime/oti/j9accessbarrier.h
Outdated
@@ -195,13 +197,15 @@ typedef struct J9IndexableObject* mm_j9array_t; | |||
) | |||
#define J9STATIC__PRE_OBJECT_STORE(vmThread, clazz, address, value) \ | |||
((void)0, \ | |||
(OMR_GC_ALLOCATION_TYPE_SEGREGATED == (J9VMTHREAD_JAVAVM((vmThread)))->gcAllocationType) ? \ | |||
((OMR_GC_WRITE_BARRIER_TYPE_SATB == javaVM->gcWriteBarrierType) || \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no javaVM directly available here
runtime/oti/j9accessbarrier.h
Outdated
J9VMTHREAD_JAVAVM((vmThread))->memoryManagerFunctions->J9MetronomeWriteBarrierJ9ClassStore((vmThread), J9VM_J9CLASS_TO_HEAPCLASS((clazz)), (j9object_t*)(address), (j9object_t)(value)) : \ | ||
(void)0 \ | ||
) | ||
#define J9STATIC__PRE_OBJECT_STORE_VM(javaVM, object, address, value) \ | ||
((void)0, \ | ||
(OMR_GC_ALLOCATION_TYPE_SEGREGATED == javaVM->gcAllocationType) ? \ | ||
((OMR_GC_WRITE_BARRIER_TYPE_SATB == (J9VMTHREAD_JAVAVM((vmThread)))->gcWriteBarrierType) || \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is not vmThread available here, but javamVM, what is exactly you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I finally fixed all the typos in this file! thank you!
The failure in travis-ci build is unrelated and has been fixed by #2649 , so the only way to pass it rebase forward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I have a few failures in my personal build, investigating. will update once it's fixed. thanks! |
I just marked this WIP pending further updates from @ebadyano to help prevent an accidental merge. |
Jenkins test sanity xLinux jdk8 |
b7f48e2
to
ed394d6
Compare
Jenkins test sanity xLinux jdk8 |
SATB barrier is used in metronome gc policy. We would like to enable it for other gc polices under option -Xgc:snapshotAtTheBeginningBarrier. These are initial changes to add the option and start decoupling barrier code from metronome gc. Signed-off-by: Evgenia Badyanova <[email protected]>
Jenkins test sanity xLinux jdk8 |
{ | ||
return ((_extensions->configuration->isSnapshotAtTheBeginningBarrierEnabled()) && | ||
(!_extensions->sATBBarrierRememberedSet->isGlobalFragmentIndexPreserved(env))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra brackets are really needed when it's not so obvious which operator has priority. I believe that for most people there is no dilemma that -> operator is among the highest. But you can leave it as is, unless you end up changing the code again.
@@ -47,7 +48,9 @@ class MM_StandardAccessBarrier : public MM_ObjectAccessBarrier | |||
#if defined(J9VM_GC_GENERATIONAL) | |||
MM_GenerationalAccessBarrierComponent _generationalAccessBarrierComponent; /**< Generational Component of Access Barrier */ | |||
#endif /* J9VM_GC_GENERATIONAL */ | |||
|
|||
#if defined(OMR_GC_REALTIME) | |||
bool _doubleBarrierActive; /**< Global indicator that the double barrier is active. New threads will be set to double barrier mode if this falg is true. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet, I was going to see where to initialize it once we start working on the code for doublebarrier (in the follow up change), the code is guarded by isSnapshotAtTheBeginningBarrierEnabled option so should cause any issues for the existing gc policies
jenkins test sanity zlinux jdk8 |
@@ -274,7 +274,7 @@ initializeReferenceArrayCopyTable(J9ReferenceArrayCopyTable *table) | |||
table->forwardReferenceArrayCopyWithCheckIndex[j9gc_modron_wrtbar_cardmark] = forwardReferenceArrayCopyWithCheckAndAlwaysWrtbarIndex; | |||
table->forwardReferenceArrayCopyWithCheckIndex[j9gc_modron_wrtbar_cardmark_incremental] = forwardReferenceArrayCopyWithCheckAndAlwaysWrtbarIndex; | |||
table->forwardReferenceArrayCopyWithCheckIndex[j9gc_modron_wrtbar_cardmark_and_oldcheck] = forwardReferenceArrayCopyWithCheckAndAlwaysWrtbarIndex; | |||
table->forwardReferenceArrayCopyWithCheckIndex[j9gc_modron_wrtbar_realtime] = forwardReferenceArrayCopyWithCheckAndAlwaysWrtbarIndex; | |||
table->forwardReferenceArrayCopyWithCheckIndex[j9gc_modron_wrtbar_satb] = forwardReferenceArrayCopyWithCheckAndAlwaysWrtbarIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this table should be expanded for satb and oldcheck too (fine for future changes)
SATB barrier is used in metronome gc policy. We would like to enable it
for other gc polices under option -Xgc:snapshotAtTheBeginningBarrier.
These are initial changes to add the option and start decoupling barrier
code from metronome gc.
Signed-off-by: Evgenia Badyanova [email protected]