Hi Ioi,
> On Jul 8, 2016, at 5:47 PM, Ioi Lam <[email protected]> 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 <[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
>>>>>
>>>>>
>
