RFR: 8141341: CDS should be disabled if JvmtiExport::should_post_class_file_load_hook() is true
Ioi Lam
ioi.lam at oracle.com
Thu Jul 14 12:47:11 UTC 2016
Hi Jiangli,
I think these are not necessary in systemDictionary.cpp, as the same
condition is checked in the "inner" load_share_class() function.
1249 // Don't load shared class when
JvmtiExport::should_post_class_file_load_hook()
1250 // is enabled since posting CFLH is not supported when loading
shared class.
1251 if (!JvmtiExport::should_post_class_file_load_hook()) {
The rest looks OK to me.
Thanks
- Ioi
On 7/13/16 5:06 PM, Jiangli Zhou wrote:
> Here is the updated webrev:
>
> http://cr.openjdk.java.net/~jiangli/8141341/webrev.01/
>
> Following is the summary of the VM behavior under difference conditions. Since this is to temporary disable sharing for CFLH, I choose to use the solution that’s least disruptive (hopefully).
> Agent specified via command line:
> With -Xshare:on, report error and exit.
> Checks are added in Meatspace::global_initialize() and Threads::create_vm(). The check in Meatspace::global_initialize() detects early enabling of JvmtiExport::should_post_class_file_load_hook(). The check added in Threads::create_vm() is to detect the enabling of JvmtiExport::should_post_class_file_load_hook() later during the VM initialization.
> With -Xshare:auto, disallow loading shared class
> If JvmtiExport::should_post_class_file_load_hook() is enabled before mapping shared archive and initialize_preloaded_classes() (init_globals()), disable sharing entirely. That is ensured by the check added in Meatspace::global_initialize().
> If JvmtiExport::should_post_class_file_load_hook() is enabled after init_globals(), the String class is loaded from the shared archive, only disallow loading shared class but shared Strings can still be used. Ensured by checks added in load_shared_class().
> Agent is attached:
> With -Xshare:on and -Xshare:auto
> Disallow loading shared class after JvmtiExport::should_post_class_file_load_hook() is enabled. Shared Strings can still be used because the String class is already loaded from the shared archive during VM initialization.
> Karen, I moved cds_address into the ‘if (UseSharedSpaces)’ as you suggested. There are code after the
> 'if (UseSharedSpaces)’ that should be executed as part of the ‘else’ case of ‘if (DumpSharedSpaces)’. So left the #if INCLUDE_CDS unchanged for ‘if (DumpSharedSpaces)’.
>
> Serguei, I added ‘delete mapinfo’ after FileMapInfo::fail_continue.
>
> Thanks,
> Jiangli
>
>
>
>> On Jul 8, 2016, at 6:14 PM, Jiangli Zhou <jiangli.zhou at Oracle.COM> wrote:
>>
>> Hi Ioi,
>>
>>> On Jul 8, 2016, at 5:47 PM, Ioi Lam <ioi.lam at 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.kinnear at 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.lam at oracle.com> wrote:
>>>>>>
>>>>>> (Adding serviceability-dev at 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
>>>>>>>
>>>>>>>
More information about the hotspot-runtime-dev
mailing list