On Thu, Jun 14, 2018 at 9:04 PM, Thomas Stüfe <thomas.stu...@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. > 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. > > Best Regards, Thomas > > On Thu, Jun 14, 2018 at 4:26 PM, Volker Simonis > <volker.simo...@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.
