[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8141341: CDS should be disabled if JvmtiExport::should_post_class_file_load_hook() is true
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2016-07-09 4:39:25
Message-ID: 57807FFD.1030100 () oracle ! com
[Download RAW message or body]

Hi Jiangli,

Ok, I'll wait for the next webrev version.

Thanks,
Serguei


On 7/8/16 18:04, Jiangli Zhou wrote:
> 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, serguei.spitsyn@oracle.com 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 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.
> > > 
> > > 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


[prev in list] [next in list] [prev in thread] [next in thread]