On Tue, Jun 19, 2018 at 9:25 AM, David Holmes <david.hol...@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.hol...@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. 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.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. >>>>>> >>>>> >>> >
