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
Disable runtime assumption table on JITServer #9189
Conversation
|
9d0508a
to
e89e865
Compare
|
|
67e71ba
to
6ba79f7
Compare
Ready for review. |
TR_RuntimeAssumptionTable * rat = compInfo->getPersistentInfo()->getRuntimeAssumptionTable(); | ||
if (rat) | ||
if (rat | ||
#if defined(J9VM_OPT_JITSERVER) |
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 we take this path at the server something is wrong.
Thus, i would rather place an ASSERT_FATAL here.
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 do take this path at the server. Investigating what's wrong here.
@@ -3550,9 +3555,6 @@ static bool updateCHTable(J9VMThread * vmThread, J9Class * cl) | |||
TR_PersistentCHTable * table = 0; | |||
if (TR::Options::getCmdLineOptions()->allowRecompilation() | |||
&& !TR::Options::getCmdLineOptions()->getOption(TR_DisableCHOpts) | |||
#if defined(J9VM_OPT_JITSERVER) | |||
&& compInfo->getPersistentInfo()->getRemoteCompilationMode() != JITServer::SERVER |
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.
Why is this code removed. The idea is that, we don't want to update the CHTable at the server because it is not used for compilation anyway.
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 persistentMemory->getPersistentInfo()->getRuntimeAssumptionTable()->init()
is the only place where we call updateCHTable()
at the server. Since that's disabled, updateCHTable()
is no longer called at the server thus doesn't need a guard.
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.
To be sure let's add an ASSERT_FATAL
Path to
|
So the hook was triggered by something from the "Unsafe" class from the sun.misc package (primarily designed for internal use within the core libraries). When the static VMINLINE void
putObject(J9VMThread *currentThread, MM_ObjectAccessBarrierAPI *objectAccessBarrier, j9object_t object, UDATA offset, bool isVolatile, j9object_t *value)
{
if (VM_VMHelpers::objectIsArray(currentThread, object)) {
UDATA index = convertOffsetToIndex(currentThread, offset, logFJ9ObjectSize(currentThread));
objectAccessBarrier->inlineIndexableObjectStoreObject(currentThread, object, index, *value, isVolatile);
} else if (offset & J9_SUN_STATIC_FIELD_OFFSET_TAG) {
/* Static field */
J9Class *fieldClass = J9VM_J9CLASS_FROM_HEAPCLASS(currentThread, object);
if (J9_ARE_ANY_BITS_SET(offset, J9_SUN_FINAL_FIELD_OFFSET_TAG)) {
VM_VMHelpers::reportFinalFieldModified(currentThread, fieldClass);
}
void *valueAddress = (void*)((UDATA)fieldClass->ramStatics + (offset & ~(UDATA)J9_SUN_FIELD_OFFSET_MASK));
objectAccessBarrier->inlineStaticStoreObject(currentThread, fieldClass, (j9object_t*)valueAddress, *value, isVolatile);
} else {
/* Instance field */
objectAccessBarrier->inlineMixedObjectStoreObject(currentThread, object, offset, *value, isVolatile);
}
} |
Ready for another review |
@@ -3136,6 +3136,15 @@ void jitMethodBreakpointed(J9VMThread * vmThread, J9Method *j9method) | |||
*/ | |||
void jitIllegalFinalFieldModification(J9VMThread *currentThread, J9Class *fieldClass) | |||
{ | |||
#if defined(J9VM_OPT_JITSERVER) |
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.
compInfo is not defined at this point
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 got the jitConfig like below:
J9JITConfig * jitConfig = currentThread->javaVM->jitConfig;
Maybe it compiles because there is a global somewhere.
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.
Good eye! I can't believe that compiled.
@@ -3550,9 +3555,6 @@ static bool updateCHTable(J9VMThread * vmThread, J9Class * cl) | |||
TR_PersistentCHTable * table = 0; | |||
if (TR::Options::getCmdLineOptions()->allowRecompilation() | |||
&& !TR::Options::getCmdLineOptions()->getOption(TR_DisableCHOpts) | |||
#if defined(J9VM_OPT_JITSERVER) | |||
&& compInfo->getPersistentInfo()->getRemoteCompilationMode() != JITServer::SERVER |
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.
To be sure let's add an ASSERT_FATAL
jenkins compile plinuxjit jdk11 |
JITServer should not create any runtime assumptions, instead it should buffer them and send them to the client. In order to catch all mistakes, it's better to disable RuntimeAssumptionTable on JITServer so any misuses will be caught by crashes. Issue: eclipse-openj9#8029 Signed-off-by: Harry Yu <harryyu1994@gmail.com>
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
jenkins test sanity plinuxjit,xlinuxjit jdk11 |
JITServer should not create any runtime assumptions, instead it
should buffer them and send them to the client. In order to catch
all mistakes, it's better to disable RuntimeAssumptionTable on JITServer
so any misuses will be caught by crashes.
Issue: #8029
Signed-off-by: Harry Yu harryyu1994@gmail.com