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

[Volker Simonis]

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).

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?

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.
>>>>
>>>
>