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

Tue Jun 19 18:10:24 UTC 2018

> On Jun 19, 2018, at 5:21 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
> 
> 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.

For the classLoaderExt.cpp bug, we could use a private function, ClassLoaderExt::disable_shared_platform_and_app_classes, which does the logic in the original ClassLoaderExt::append_boot_classpath #INCLUDE_CDS. ClassLoaderExt::append_boot_classpath could be defined in classLoaderExt.hpp as:

void disable_shared_platform_and_app_classes() NOT_CDS_RETURN;

void append_boot_classpath(ClassPathEntry* new_entry) {
  disable_shared_platform_and_app_classes();
  ClassLoader::add_to_boot_append_entries(new_entry);
}

The new guard can be removed from jvmtiEnv.cpp with those. Reducing CDS specifics from general code probably is cleaner.

Thanks,
Jiangli

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