Hi Karen, Thank you for the feedback. I think Ioi and your comments regarding the dynamically attached agent is very reasonable. I will rework the changes.
Thanks, Jiangli > On Jul 8, 2016, at 7:55 AM, Karen Kinnear <[email protected]> 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 <[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 >>> >>> >> >
