Looks good to me, Volker. Thank you for fixing the tests. ..Thomas
On Mon, Jun 18, 2018 at 6:04 PM, Volker Simonis <volker.simo...@gmail.com> wrote: > On Mon, Jun 18, 2018 at 8:17 AM, David Holmes <david.hol...@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. >>> >>
