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

Jiangli Zhou jiangli.zhou at oracle.com
Tue Jun 19 18:10:24 UTC 2018

On Jun 19, 2018, at 5:21 AM, Volker Simonis <volker.simonis at gmail.com> wrote:

On Tue, Jun 19, 2018 at 9:25 AM, David Holmes <david.holmes at 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.holmes at oracle.com> wrote:

Hi Volker, v3 looks much cleaner - thanks. But AFAICS the change to jvmtiEnv.cpp is also not needed. ClassLoaderExt::appendbootclasspath exists regardless of INCLUDECDS but operates differently (just calling ClassLoader::addtobootappendentries).

That's not entirely true because the whole compilation unit (i.e. classLoaderExt.cpp) which contains 'ClassLoaderExt::appendbootclasspath()' 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 INCLUDECDS guards! I suspect it should not be excluded. So I can either move the whole implementation of 'ClassLoaderExt::appendbootclasspath()' 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 'INCLUDECDS' or they only use functions defined in classLoaderExt.hpp. The single remaining reference from jvmtiEnv.cpp has been guarded with 'INCLUDECDS' 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.

For the classLoaderExt.cpp bug, we could use a private function, ClassLoaderExt::disable_shared_platform_and_app_classes, which does the logic in the original ClassLoaderExt::append_boot_classpath #INCLUDE_CDS. ClassLoaderExt::append_boot_classpath could be defined in classLoaderExt.hpp as:

void disable_shared_platform_and_app_classes() NOT_CDS_RETURN;

void append_boot_classpath(ClassPathEntry* new_entry) { disable_shared_platform_and_app_classes(); ClassLoader::add_to_boot_append_entries(new_entry); }

The new guard can be removed from jvmtiEnv.cpp with those. Reducing CDS specifics from general code probably is cleaner.

Thanks, Jiangli

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.holmes at 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, CDSONLY/NOTCDS 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 INCLUDECDS in the ifdefs not INCLUDECDSJAVAHEAP. 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 isarchiveobject(oop p) NOTCDSJAVAHEAPRETURN(false); ./hotspot/share/classfile/stringTable.hpp: static oop createarchivedstring(oop s, Thread* THREAD) NOTCDSJAVAHEAPRETURN(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 CDSONLY 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.

More information about the build-dev mailing list