On Jul 8, 2016, at 7:55 AM, Karen Kinnear <karen.kinn...@oracle.com> wrote:
Jiangli,
Thank you so much for picking up this bug fix, I very much appreciate that.
I agree with not quitting the vm on agent attach.
I actually think we also want the agent attach to succeed - see below.
On Jul 7, 2016, at 9:56 PM, Ioi Lam <ioi....@oracle.com> wrote:
(Adding serviceability-dev@openjdk.java.net)
Hi Jiangli,
I think the two blocks of
if (RequireSharedSpaces) {
tty->print_cr("An error has occurred while loading shared class.");
tty->print_cr("Tool agent requires sharing to be disabled.");
vm_exit(1);
}
should be removed.
I agree with this part. I was expecting the else to return a null
instanceklasshandle as if the shared class was not found,
but no forcing of using the shared spaces if there is an agent.
If the agent is specified in the command line, the JVM start would be aborted.
This is already handled by your changes to src/share/vm/memory/metaspace.cpp
Yes, here it makes sense to do the fail_continue such that RequireSharedSpaces
will prevent starting the JVM.
If the agent is attached later, after the VM has started, I think we should not
quit the VM.
Totally agree - we should not quit the VM.
Consider this scenario -- a production server could be running for a long time
without issues using -Xshare:on, then when the user attaches a diagnostic agent
the JVM suddenly quits. I think it's better to cause the agent attachment to
fail with something like
jvmtiError
JvmtiEnv::SetEventNotificationMode(jvmtiEventMode mode, jvmtiEvent event_type,
jthread event_thread, ...) {
...
if (event_type == JVMTI_EVENT_CLASS_FILE_LOAD_HOOK && enabled) {
+ if (RequireSharedSpaces && !universe::is_bootstraping()) {
+ tty->print_cr("Tool agent that requires JVMTI_EVENT_CLASS_FILE_LOAD_HOOK
cannot be dynamically loaded after JVM has started");
+ return JVMTI_ERROR_ILLEGAL_ARGUMENT;
+ }
record_class_file_load_hook_enabled();
}
…
}
I was expecting the vm to continue running, letting the agent attach (customers
really want to use these).
Jiangli - if I read your code correctly, that is what you do if UseSharedSpaces
but not if RequireSharedSpaces.
I would propose that you do the same behavior for both cases once we are up and
running - i.e. get rid of the
two blocks Ioi suggested removing and just fall through as if the file was not
found in the archive regardless of RequireSharedSpaces.
Thanks
- Ioi
Jiangli:
Minor code review comments:
metaspace.cpp line 3181: CFHL-> CFLH
metaspace.cpp:
I appreciated your moving the if (UseSharedSpaces) into the #INCLUDE_CDS.
I think it would make sense to do a bit more of that - e.g. cds_address
appears to only be
use locally, so it also could be inside the #if INCLUDE_CDS.
Would it make sense to have the entire if (DumpSharedSpaces) etc. all be
inside a single #if INCLUDE_CDS?
thanks,
Karen
On 7/7/16 6:08 PM, Jiangli Zhou wrote:
Please review the following webrev that disables CDS when
JvmtiExport::should_post_class_file_load_hook() is enabled at runtime.
webrev: http://cr.openjdk.java.net/~jiangli/8141341/webrev.00/
bug: JDK-8141341 <https://bugs.openjdk.java.net/browse/JDK-8141341>
With -Xshare:on
If -Xshare:on is used and JvmtiExport::should_post_class_file_load_hook() is
required, the VM now reports "Tool agent requires sharing to be disabled” and
terminates. This is the same behavior as jdk8.
With -Xshare:auto
The change in meatspace.cpp is to detect the case where
JvmtiExport::should_post_class_file_load_hook() is enabled very early during
JVM initialization. In that case, we disable CDS entirely, including all shared
classes, symbols, and Strings objects.
The change in systemDictionary.cpp is to detect the cases where
JvmtiExport::should_post_class_file_load_hook() is enabled late, which include
agent dynamically attached at runtime. In those cases, JVM does not allow
loading classes from the shared archive. The shared symbols and Strings can
still be used.
Thanks,
Jiangli
