Add option to disable SCC after CRIU restore #16921
Conversation
|
@hangshao0 |
runtime/shared/shrclssup.c
Outdated
| @@ -276,6 +276,8 @@ IDATA J9VMDllMain(J9JavaVM* vm, IDATA stage, void* reserved) | |||
| } | |||
| vm->sharedCacheAPI->runtimeFlags = runtimeFlags; | |||
| } | |||
|
|
|||
| vm->sharedCacheAPI->xShareClassCacheDisabledOnCRIURestore = FALSE; | |||
There was a problem hiding this comment.
Do you need to set xShareClassCacheDisabledOnCRIURestore to FALSE ? Line 53 memset the whole sharedCacheAPI struct to 0 already.
| J9JavaVM *vm = currentThread->javaVM; | ||
|
|
||
| if (NULL != vm->checkpointState.restoreArgsList) { | ||
| if (FIND_AND_CONSUME_ARG(vm->checkpointState.restoreArgsList, EXACT_MATCH, VMOPT_XSHARECLASSES_DISABLEONRESTORE, NULL) >= 0) { |
There was a problem hiding this comment.
There should be a NULL check of vm->sharedClassConfig. If it is NULL, the shared class is already disabled.
runtime/vm/CRIUHelpers.cpp
Outdated
| * @param[in] currentThread vmThread token | ||
| * @param[in] userData J9InternalHookRecord pointer | ||
| * | ||
| * @return BOOLEAN TRUE if no error, otherwise FALSE |
There was a problem hiding this comment.
I see this function can only return TRUE.
| void | ||
| j9shr_disableSharedClassCacheForCriuRestore(J9JavaVM* vm) | ||
| { | ||
| vm->sharedClassConfig->runtimeFlags |= J9SHR_RUNTIMEFLAG_DENY_CACHE_ACCESS | J9SHR_RUNTIMEFLAG_DENY_CACHE_UPDATES; |
There was a problem hiding this comment.
Can you check the APIs checking these 2 flags (J9SHR_RUNTIMEFLAG_DENY_CACHE_ACCESS and J9SHR_RUNTIMEFLAG_DENY_CACHE_UPDATES), we may need to update the return code. e.g.
openj9/runtime/shared_common/shrinit.cpp
Line 1820 in 03967d8
The comment of this API says Return -1 in the case of error. But if xShareClassCacheDisabledOnCRIURestore is TRUE, it should not be an error case. It should behave as if the data is not in the shared cache and return 0.
There was a problem hiding this comment.
I just checked every usage of those two flags and I didn't find anything else that was incorrect, so I'll just fix that one.
There was a problem hiding this comment.
Fixed that function in 86ce34e
| if (vm->sharedCacheAPI->xShareClassCacheDisabledOnCRIURestore) { | ||
| return 0; | ||
| } else { | ||
| return -1; |
There was a problem hiding this comment.
You need to add trace point Trc_SHR_INIT_findSharedData_exit_Noop before these 2 returns.
There was a problem hiding this comment.
Approve assuming trace points are added.
| j9shr_disableSharedClassCacheForCriuRestore(J9JavaVM* vm) | ||
| { | ||
| vm->sharedClassConfig->runtimeFlags |= J9SHR_RUNTIMEFLAG_DENY_CACHE_ACCESS | J9SHR_RUNTIMEFLAG_DENY_CACHE_UPDATES; | ||
| vm->sharedCacheAPI->xShareClassCacheDisabledOnCRIURestore = TRUE; |
There was a problem hiding this comment.
@dsouzai JIT can check this variable vm->sharedCacheAPI->xShareClassCacheDisabledOnCRIURestore. It is TRUE if vm->sharedClassConfig is not NULL and "-Xshareclasses:disableOnRestore" presents. (If vm->sharedClassConfig is NULL, SCC is already disabled)
|
@ehrenjulzert please rebase these changes |
|
@tajila Are the CRIU related options documented ? If yes, a doc issue should be created for the new option added here. |
for eclipse-openj9#16868 Signed-off-by: Ehren Julien-Neitzert <ehren.julien-neitzert@ibm.com>
|
Just rebased |
|
jenkins compile win jdk8 |
|
jenkins test sanity xlinux jdk17 |
|
@ehrenjulzert Please make a PR for 0.38 |
|
0.38 PR: #16921 |
|
Whoops, yes that's correct |
for #16868