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

[Thomas Stüfe]

Hi Volker,



On Fri, Jun 15, 2018 at 10:05 AM, Volker Simonis
<volker.simonis at gmail.com> wrote:
> On Thu, Jun 14, 2018 at 9:04 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>> Hi Volker,
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/make/autoconf/hotspot.m4.udiff.html
>>
>> Seems like a roundabout way to have a platform specific default value.
>>
>> Why not determine a default value beforehand:
>>
>> if test "x$OPENJDK_TARGET_OS" = "xaix"; then
>>   ENABLE_CDS_DEFAULT="false"
>> else
>>   ENABLE_CDS_DEFAULT=true"
>> fi
>>
>> AC_ARG_ENABLE([cds], [AS_HELP_STRING([--enable-cds@<:@=yes/no/auto@:>@],
>>  [enable class data sharing feature in non-minimal VM. Default is
>> ${ENABLE_CDS_DEFAULT}.])])
>>
>> and so on?
>>
>
> I've just followed the pattern used for '--enable-aot' right above the
> code I changed.
>
> Moreover, I don't think that we would save any code because we would
> still have to check for AIX in the '--enable-cds=yes' case. Also, the
> new reporting added later in the file (see "AC_MSG_CHECKING([if cds
> should be enabled])" seems easier to me without the extra default
> value. So if you don't mind I'd prefer to leave it as is.
>

I just think that having three options (on/off/auto) is confusing.
Okay, I still think a platform dependent default value would be
cleaner, but I can live with auto="yes, if possible".

>> See also what we did for "8202325: [aix] disable warnings-as-errors by default".
>>
>> --
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/src/hotspot/share/classfile/javaClasses.cpp.udiff.html
>>
>> Here, do we really need to exclude this from compiling,
>> DumpSharedSpaces = false is not enough?
>>
>
> Yes, we need it because the excluded code references methods (e.g.
> 'StringTable::create_archived_string()') which are not compiled into
> libjvm.so if we disable CDS.
>

Are you really sure?

Both MetaspaceShared::is_archive_object() and
StringTable::create_archived_string() are available outside CDS, the
latter explicitly returns NULL if CDS is not enabled at build time:
NOT_CDS_JAVA_HEAP_RETURN_(NULL);

I also just built a Linux vm without CDS, and it compiles without
problems without the #ifdef. But maybe AIX is different.

---

But all this is idle nitpicking, so I leave it up to you if you change
anything. The change is good in its current form to me and I do not
need another webrev.

Best Regards, Thomas

>>
>> Best Regards, Thomas
>>
>> On Thu, Jun 14, 2018 at 4:26 PM, Volker Simonis
>> <volker.simonis at gmail.com> 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.