Hi Ioi, > On Jul 8, 2016, at 5:47 PM, Ioi Lam <ioi....@oracle.com> wrote: > > Hi Jiangli, > > I agree what what Karen has said. > > If the user runs with -Xshare:on, but later attaches an agent, we just > pretend that the user had specified -Xshare:auto -- allow the agent to > receive all CFLH events, and stop loading from the shared archive. > > We should probably print out a warning message during the first > load_share_class() call after the agent attachment, saying that CDS has been > disabled due to attaching agent.
Agreed. > > Also, maybe this check can also be removed? > > 1247 instanceKlassHandle SystemDictionary::load_shared_class( > 1248 Symbol* class_name, Handle class_loader, TRAPS) { > 1249 if (!JvmtiExport::should_post_class_file_load_hook()) { <<<<<<<<<<<<<<< > > Since the same check is already done in the "inner" load_shared_class() > method. I ran into issue with the find_shared_class() before the inner load_shared_class() is called earlier. That’s why the check was done in both load_shared_class. With updated approach, we might no longer need the check here. Thanks, Jiangli > > Thanks > - Ioi > > > On 7/8/16 11:18 AM, Jiangli Zhou wrote: >> 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 <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 >>>>> >>>>> >