Enable JITInlineFieldwatch when option specified #4253
Conversation
This change enables JIT inlined field watch on X86 when -XX:+JITInlineWatches is specified in the Java command line. Signed-off-by: Yi Zhang <yizhang@ca.ibm.com>
| #if !defined(TR_HOST_X86) | ||
| //The bit is set when -XX:+JITInlineWatches is specified | ||
| if (J9_ARE_ANY_BITS_SET(javaVM->extendedRuntimeFlags, J9_EXTENDED_RUNTIME_JIT_INLINE_WATCHES)) | ||
| TR_ASSERT_FATAL(false, "this platform doesn't support JIT inline field watch"); |
There was a problem hiding this comment.
I'm not sure a fatal assert and crash is the right thing to do - isn't there a more graceful way to fail?
There was a problem hiding this comment.
It's a new option for users to experiment with and will not be publicized so It's probably better to keep the assertion here and let it fail fast in our testing environment.
|
@gacholio Currently we assert on platforms not supporting field watch and it's better if we can fall back to old field watch without crashing. Is there a place in VM we can do that? |
|
If we had a build flag in the spec, we could do it. I'm not sure it's worth the effort, as I don't plan to publicize the option to enable field watch (once it's working on all platforms, we'll remove the old code). @DanHeidinga Any thoughts? I'm fine with the assertion in the short term. |
|
I'm on board with failing to start the JVM if an invalid option is specified. An assert feels like a strange way to do that. Isn't there an existing way in the JIT to fail to start due to invalid options? Also, Thinking about this further, we should only check for the |
|
We can do that (though the options are soon not going to be ignored, iirc). Better that we fail early with a vaguely useful message than assert. |
|
I misread this PR. It's not adding the option, only testing the result. In that case, asserting is fine as we should never enable the bit on a platform that doesn't support the option. |
|
@cathyzhyi If you remove your check and assert, you can add |
|
For performance testing, asserting is better than silently ignoring the option. |
|
Looks like the final decision is to leave the assert the way it is. |
|
ok so I'm fine with leaving the assert in the IT given the comments above |
|
Jenkins test sanity xlinux,win,plinux jdk8 |
This change enables JIT inlined field watch on X86 when
-XX:+JITInlineWatches is specified in the Java command line.
Signed-off-by: Yi Zhang yizhang@ca.ibm.com