RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

Tue Jun 19 12:21:54 UTC 2018

On Tue, Jun 19, 2018 at 9:25 AM, David Holmes <david.holmes at oracle.com> wrote:
> Hi Volker,
>
> On 19/06/2018 4:50 PM, Volker Simonis wrote:
>>
>> On Tue, Jun 19, 2018 at 6:54 AM, David Holmes <david.holmes at oracle.com>
>> wrote:
>>>
>>> Hi Volker,
>>>
>>> v3 looks much cleaner - thanks.
>>>
>>> But AFAICS the change to jvmtiEnv.cpp is also not needed.
>>> ClassLoaderExt::append_boot_classpath exists regardless of INCLUDE_CDS
>>> but
>>> operates differently (just calling
>>> ClassLoader::add_to_boot_append_entries).
>>>
>>
>> That's not entirely true because the whole compilation unit (i.e.
>> classLoaderExt.cpp) which contains
>> 'ClassLoaderExt::append_boot_classpath()' is excluded from the
>> compilation if CDS is disabled (see make/hotspot/lib/JvmFeatures.gmk).
>
>
> Hmmm. There's a CDS bug there. Either classLoaderExt.cpp should not be
> excluded from a non-CDS build, or it should not contain any INCLUDE_CDS
> guards! I suspect it should not be excluded.
>
>> So I can either move the whole implementation of
>> 'ClassLoaderExt::append_boot_classpath()' into classLoaderExt.hpp in
>> which case things would work as you explained and my changes to
>> jvmtiEnv.cpp could be removed or leave the whole code and change as
>> is. Please let me know what you think?
>
>
> In the interest of moving forward you can push what you have and I will file
> a bug against CDS to sort out classLoaderExt.cpp.
>

Thanks! As the current version also passed the submit-repo tests I've pushed it.

Regarding classLoaderExt.cpp, I think it is OK to exclude it for
non-CDS builds. If my IDE doesn't cheat on me (see [1]),
ClassLoaderExt is mostly used from other CDS-only files
(classListParser.cpp, systemDictionaryShared.cpp, filemap.cpp,
metaspaceShared.cpp). The only references from non-CDS files are from
classLoader.cpp an jvmtiEnv.cpp. The ones from classLoader.cpp are all
guarded with 'INCLUDE_CDS' or they only use functions defined in
classLoaderExt.hpp. The single remaining reference from jvmtiEnv.cpp
has been guarded with 'INCLUDE_CDS' by my change.

I think it is a matter of taste if we leave this as is or move the
offending function from classLoaderExt.cpp to classLoaderExt.hpp and
remove the new guard from jvmtiEnv.cpp.

Regards,
Volker

[1] http://cr.openjdk.java.net/~simonis/webrevs/2018/ClassLoaderExt.html

> Thanks,
> David
>
>
>> Regards,
>> Volker
>>
>>> Thanks,
>>> David
>>>
>>>
>>> On 19/06/2018 2:04 AM, Volker Simonis wrote:
>>>>
>>>>
>>>> On Mon, Jun 18, 2018 at 8:17 AM, David Holmes <david.holmes at oracle.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi Volker,
>>>>>
>>>>> src/hotspot/share/runtime/globals.hpp
>>>>>
>>>>> This change should not be needed! We do minimal VM builds without CDS
>>>>> and
>>>>> we
>>>>> don't have to touch the UseSharedSpaces defaults (unless recent change
>>>>> have
>>>>> broken this - in which case that needs to be addressed in its own
>>>>> right!)
>>>>>
>>>>
>>>> Yes, you're right, CDS_ONLY/NOT_CDS isn't really required here,
>>>> because UseSharedSpaces is reseted later on at the end of
>>>> Arguments::parse(). I just thought it would be cleaner to disable it
>>>> statically, if the VM doesn't support it. But anyway I don't really
>>>> mind and I've reverted that change in globals.hpp.
>>>>
>>>>> src/hotspot/share/classfile/javaClasses.cpp
>>>>>
>>>>> AFAICS you should be using INCLUDE_CDS in the ifdefs not
>>>>> INCLUDE_CDS_JAVA_HEAP. But again I'm unclear (as was Thomas) why this
>>>>> should
>>>>> be needed as we have not needed it before. As Thomas notes we have:
>>>>>
>>>>> ./hotspot/share/memory/metaspaceShared.hpp:  static bool
>>>>> is_archive_object(oop p) NOT_CDS_JAVA_HEAP_RETURN_(false);
>>>>> ./hotspot/share/classfile/stringTable.hpp:  static oop
>>>>> create_archived_string(oop s, Thread* THREAD)
>>>>> NOT_CDS_JAVA_HEAP_RETURN_(NULL);
>>>>>
>>>>> so these methods should be defined when CDS is not available.
>>>>>
>>>>
>>>> Thomas and you are right. Must have been a mis-configuration on AIX
>>>> where I saw undefined symbols at link time. I've removed the ifdefs
>>>> from javaClasses.cpp now.
>>>>
>>>> Finally, I've also wrapped all the FileMapInfo fields in vmStructs.cpp
>>>> into CDS_ONLY macros as suggested by Jiangli because the really only
>>>> make sense for a CDS-enabled VM.
>>>>
>>>> Here's the new webrev:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965.v3/
>>>>
>>>> Please let me know if you think there's still something missing.
>>>>
>>>> Regards,
>>>> Volker
>>>>
>>>>
>>>>> ??
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 15/06/2018 12:26 AM, Volker Simonis wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> can I please have a review for the following fix:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204965
>>>>>>
>>>>>> CDS does currently not work on AIX because of the way how we
>>>>>> reserve/commit memory on AIX. The problem is that we're using a
>>>>>> combination of shmat/mmap depending on the page size and the size of
>>>>>> the memory chunk to reserve. This makes it impossible to reliably
>>>>>> reserve the memory for the CDS archive and later on map the various
>>>>>> parts of the archive into these regions.
>>>>>>
>>>>>> In order to fix this we would have to completely rework the memory
>>>>>> reserve/commit/uncommit logic on AIX which is currently out of our
>>>>>> scope because of resource limitations.
>>>>>>
>>>>>> Unfortunately, I could not simply disable CDS in the configure step
>>>>>> because some of the shared code apparently relies on parts of the CDS
>>>>>> code which gets excluded from the build when CDS is disabled. So I
>>>>>> also fixed the offending parts in hotspot and cleaned up the configure
>>>>>> logic for CDS.
>>>>>>
>>>>>> Thank you and best regards,
>>>>>> Volker
>>>>>>
>>>>>> PS: I did run the job through the submit forest
>>>>>> (mach5-one-simonis-JDK-8204965-20180613-1946-26349) but the results
>>>>>> weren't really useful because they mention build failures on linux-x64
>>>>>> which I can't reproduce locally.
>>>>>>
>>>>>
>>>
>