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 <[email protected]> wrote: > > (Adding [email protected]) > > 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 >> >> >
