Hi Volker,
On Fri, Jun 15, 2018 at 10:05 AM, Volker Simonis <volker.simo...@gmail.com> wrote: > 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. > 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.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.
