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

[David Holmes]

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