Hi Serguei, Thank you for looking into the changes. With Ioi and Karen’s suggestion for dynamically attached agent and -Xshare:on, I’m reworking the changes. I’m planning to remove the check below and do that in a later place. I’ll post the new webrev next week.
Thanks, Jiangli > On Jul 8, 2016, at 12:59 PM, [email protected] wrote: > > Hi Jiangli, > > > src/share/vm/memory/metaspace.cpp > > 3177 if (UseSharedSpaces) { > 3178 FileMapInfo* mapinfo = new FileMapInfo(); > 3179 > 3180 if (JvmtiExport::should_post_class_file_load_hook()) { > 3181 // Currently CDS does not support JVMTI CFHL when loading shared class. > 3182 // If JvmtiExport::should_post_class_file_load_hook is already enabled, > 3183 // just disable UseSharedSpaces. > 3184 FileMapInfo::fail_continue("Tool agent requires sharing to be > disabled."); > 3185 } else { > > > I understood, the FileMapInfo::fail_continue() will fail if the > RequireSharedSpaces is enabled which is expected. > Should the mapinfo be deallocated in the case FileMapInfo::fail_continue() is > called? > > Also, there is a comment below. > > > On 7/7/16 18:56, Ioi Lam 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. >> >> 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 >> >> If the agent is attached later, after the VM has started, I think 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(); >> } >> ... >> } > > > This is a good concern and suggestion. > We may need more sophisticated handling for this. > Sorry, I did not notice it earlier. > > There is a capability can_generate_all_class_hook_events. > We already have this in the prims/jvmtiManageCapabilities.cpp: > > // if the capability sets are initialized in the onload phase then // it > happens before class data sharing (CDS) is initialized. If it // turns out > that CDS gets disabled then we must adjust the always // capabilities. To > ensure a consistent view of the capabililties // anything we add here should > already be in the onload set. void > JvmtiManageCapabilities::recompute_always_capabilities() { if > (!UseSharedSpaces) { jvmtiCapabilities jc = always_capabilities; > jc.can_generate_all_class_hook_events = 1; always_capabilities = jc; } } If > enabled can_generate_all_class_hook_events sets the > flagJvmtiExport::can_modify_any_class() in jvmtiManageCapabilities.cpp: > JvmtiExport::set_can_modify_any_class( avail.can_generate_breakpoint_events > || avail.can_generate_all_class_hook_events); The flag > JvmtiExport::can_modify_any_class() is used here: > > char* FileMapInfo::map_region(int i) { . . . // If a tool agent is in use > (debugging enabled), we must map the address space RW if > (JvmtiExport::can_modify_any_class() || JvmtiExport::can_walk_any_space()) { > si->_read_only = false; } > > So, I'm thinking if we could just skip the CFLH events for the shared classes > if the capability can_generate_all_class_hook_events is not enabled and > continue posting the events for non-shared classes. Then we do not need to > tweak the SetEventNotificationMode() implementation. Otherwise, the fragment > above may need to be updated too. To summarize, I understood that the current > approach/plan for JDK 9 is: - Disable the CDS if detected early that the > CFLH events are enabled - Skip the CFLH events for shared classes if CFLH > is enabled late Is this right? Is it the same as in the JDK 8? Thanks, Serguei >> Thanks - Ioi 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
