RFR: JDK-8152666: The new Hotspot Build System
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Apr 7 13:54:33 UTC 2016
> I'm not sure if I'm formally allowed to be a reviewer, since I've
> wrote the absolute majority of the code myself.
The way I've done this in the past is a "Contributed-by:" line listing
all of the folks that contributed and a "Reviewed-by:" line listing all
the reviewers. Magnus, you reviewed Erik's changes and vice versa...
Dan
On 4/7/16 3:57 AM, Magnus Ihse Bursie wrote:
> On 2016-04-06 11:10, Erik Joelsson wrote:
>> Hello Dan and thank you for the review! I know it's a lot to chew
>> through.
>>
>> I have incorporated your changes and published a new webrev:
>> http://cr.openjdk.java.net/~erikj/8152666/webrev.02/
>
> I'm not sure if I'm formally allowed to be a reviewer, since I've
> wrote the absolute majority of the code myself.
>
> Nevertheless, I've looked through the webrev carefully, including the
> latest changes by you, and it looks good to me. Ship it! :-)
>
> Just a few minor comments:
>
> In compare.sh.in:
> Why the added export of DEBUG_LEVEL? I can't find any reference to it
> in the changes in compare.sh. Was it references in some earlier change
> and we missed to export it?
>
> In flags.m4/platform.m4:
> It is unfortunate that we needed to make the build/target duplication
> in this change. It makes the messy addition of the JVM_CFLAGS even
> messier. :( But then again, we've always planned a follow-up
> restructuring of the flag handling after the integration of the new
> Hotspot build system. It just got a bit more urgent.
>
> /Magnus
>
>
>>
>> On 2016-04-05 20:10, Daniel D. Daugherty wrote:
>>>
>>>
>>> > The new build supports the following variants:
>>> >
>>> > * server (C1+C2)
>>>
>>> The above "server" variant is the "tiered server". Does the new
>>> build system support the "C2 server" variant? What about the
>>> 32-bit server and 64-bit server build variants? For example,
>>> on Linux you can have:
>>>
>>> * C1/Client, 32-bit
>>> * C2/Server, 32-bit
>>> * Tiered (C1 & C2), 32-bit
>>> * C2/Server, 64-bit
>>> * Tiered (C1 + C2), 64-bit
>>>
>>> The above wide range of variants is also true for Win*.
>>>
>> There is a way to achieve this even if it's not as straight forward.
>> It's controlled through the new "jvm-feature" setting. To build a
>> completely custom set of features for a jvm, you set the
>> --with-jvm-variants=custom and then define the full feature set using
>> --with-jvm-features=compiler2,... For "server, client, core, minimal,
>> zero and zeroshark" there is a predefined set of features while the
>> custom variant has no features by default.
>>>
>>> General
>>> Please make sure all the copyrights are updated.
>>>
>> Done
>>>
>>> common/autoconf/basics.m4
>>> No comments.
>>>
>>> common/autoconf/build-performance.m4
>>> No comments.
>>>
>>> common/autoconf/buildjdk-spec.gmk.in
>>> No comments.
>>>
>>> common/autoconf/compare.sh.in
>>> No comments.
>>>
>>> common/autoconf/configure
>>> No comments.
>>>
>>> common/autoconf/configure.ac
>>> No comments.
>>>
>>> common/autoconf/flags.m4
>>> L274: SHARED_LIBRARY_FLAGS="-dynamiclib
>>> -compatibility_version 1.0.0 -current_version 1.0.0 $PICFLAG"
>>> L275: JVM_CFLAGS="$JVM_CFLAGS -fPIC"
>>>
>>> L275 is new, but seeing it next to L274 makes me wonder if
>>> $PICFLAG should be used instead of the literal '-fPIC'?
>> Fixed
>>>
>>> L303: JVM_CFLAGS="$JVM_CFLAGS -fPIC"
>>> Same question about literal '-fPIC'.
>>>
>> Not sure, leaving for now. It seems we leave the PICFLAG empty for
>> the JDK build and only add it to the hotspot build. This should be
>> addressed in a followup where we try to align flag usage more between
>> the different libraries.
>>> For most of the changes to flags.m4, I can't see how any of it
>>> relates to the new HotSpot build.
>>>
>>> Update: Now I'm wondering if this is one of those files that
>>> we typically don't review because it is auto generated.
>>> Sorry, don't remember for sure.
>> It's a file that should be reviewed, only generated-configure.sh can
>> be ignored. The majority of the changes in here are related to cross
>> compiling in the modular world. When cross compiling now, we need to
>> also build a jvm for the build platform in order to run jlink and
>> jmod when building images. With the old hotspot build, that was
>> simpler, just invoke the hotspot build with some ARCH and compiler
>> related variables set. For the rest of the JDK build, an
>> approximation of flags used was enough so the problem was never fully
>> solved.
>>
>> In the new build, we derive all the compiler options in configure so
>> I had to introduce a more proper solution. I did this by
>> parameterizing some macros in flags.m4 and platform.m4 so that we can
>> run them twice, once for the "target" toolchain" and one for the
>> "build" toolchain. These are the majority of the changes you are
>> seeing. I also removed the old hard coded "build" versions of certain
>> flag and platform variables.
>>> common/autoconf/generated-configure.sh
>>> 2642 lines changed... I think this is one of those files
>>> you're supposed to skip in build-dev review... :-|
>> Yes, please do.
>>>
>>> common/autoconf/help.m4
>>> L179: $PRINTF "Which are valid to use depends on the target
>>> platform.\n "
>>> L180: $PRINTF "%s " $VALID_JVM_FEATURES
>>> Why are there blanks after the last '\n' on L179 instead of
>>> at the beginning of L180?
>>>
>> If you do $PRINTF " %s " $VALID_JVM_FEATURES, it adds those spaces
>> between every element in VALID_JVM_FEATURES.
>>> common/autoconf/hotspot-spec.gmk.in
>>> No comments.
>>>
>>> common/autoconf/hotspot.m4
>>> L46: # Check if the specified JVM features are explicitely
>>> enabled. To be used in
>>> Typo: 'explicitely' -> 'explicitly'
>>>
>>> L59: # server: normal interpreter, and a tiered C1/C2 compiler
>>> So no support for a C2-only server config?
>>>
>>> L77: # Have the user listed more than one variant?
>>> Typo: 'Have' -> 'Has'
>>>
>> fixed
>>> common/autoconf/jdk-options.m4
>>> No comments other than to say thanks for keeping support
>>> for 'optimized' builds.
>>>
>>> common/autoconf/jdk-version.m4
>>> No comments.
>>>
>>> common/autoconf/lib-std.m4
>>> No comments.
>>>
>>> common/autoconf/libraries.m4
>>> No comments.
>>>
>>> common/autoconf/platform.m4
>>> No comments, but mind numbing amount of diffs.
>>>
>> Same explanation as for flags.m4
>>> common/autoconf/spec.gmk.in
>>> No comments.
>>>
>>> common/autoconf/toolchain.m4
>>> No comments.
>>>
>>> common/autoconf/version-numbers
>>> No comments.
>>>
>>> common/bin/compare.sh
>>> No comments.
>>>
>>> common/bin/compare_exceptions.sh.incl
>>> No comments.
>>>
>>> make/Jprt.gmk
>>> No comments.
>>>
>>> make/Main.gmk
>>> No comments other than the 'hotspot-ide-project' target
>>> looks interesting...
>>>
>> This is the replacement for the visual studio project generator. We
>> currently only support VS here.
>>> make/common/MakeBase.gmk
>>> No comments.
>>>
>>> make/common/NativeCompilation.gmk
>>> L649: else ifeq (LOW, $$($1_OPTIMIZATION))
>>> L650: $1_OPT_CFLAGS := $(C_O_FLAG_NORM)
>>> L651: $1_OPT_CXXFLAGS := $(CXX_O_FLAG_NORM)
>>> Instead of "_NORM", I was expecting "_LOW".
>>>
>>> L652: else ifeq (HIGH, $$($1_OPTIMIZATION))
>>> L653: $1_OPT_CFLAGS := $(C_O_FLAG_HI)
>>> L654: $1_OPT_CXXFLAGS := $(CXX_O_FLAG_HI)
>>> Instead of "_HI" I was expecting "_HIGH".
>>>
>> The names here were defined way back when we did build infra for the
>> JDK build. I wouldn't mind better alignment in naming the
>> optimization levels.
>>> make/jprt.properties
>>> L136: # Don't disable precompiled headers on windows. It's
>>> simply too slow.
>>> This is a surprise. Not the slowness part, but not being
>>> able to do a non-PCH JPRT build on Win*. IMHO, it's a
>>> little too much motherhood...
>>>
>> Actually, the old hotspot build does not allow disabling of PCH for
>> windows at all. The flag is simply ignored. In the new build, we
>> treat the flag the same on all platforms, so disabling precompiled
>> headers works on Windows. In the current JPRT config, we disable
>> precompiled headers on all fastdebug builds as a way of making sure
>> we aren't breaking that build configuration. We noticed a major build
>> time regression on Windows fastdebug builds in JPRT until we figured
>> out it was caused by this. Since we aren't currently disabling
>> precompiled header on Windows, I see no reason to start now. The
>> build time regression for just building hotspot is around 2m->12m.
>>> jdk/make/Import.gmk
>>> No comments.
>>>
>>> jdk/make/copy/Copy-java.base.gmk
>>> No comments.
>>>
>>> jdk/make/lib/CoreLibraries.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/BuildHotspot.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/Dist.gmk
>>> L52: define macosx_universalize
>>> I thought MacOS X universal support was going away?
>>>
>>> Update: OK, I see the mention of 8069540 ahead...
>>>
>> Yeah, we need to be binary the same as the old build for now.
>> Hopefully we can get rid of the universal stuff soon.
>>> L120: # these files are identical, and just pick one arbitrarily
>>> to use as souce.
>>> Typo: 'souce' -> 'source'
>>>
>>> L139: # This might have been defined in a custom extenstion
>>> Typo: 'extenstion' -> 'extension'
>>>
>> fixed
>>> L168: # NOTE: In the old build, this file was not copied on
>>> Windows.
>>> L169: ifneq ($(OPENJDK_TARGET_OS), windows)
>>> L170: $(eval $(call SetupCopyFiles, COPY_JVMTI_HTML, \
>>> I'm not quite sure why the jvmti.html work is done for
>>> more than a single platform.
>>>
>>> Update: Thinking about this more... I vaguely remember that
>>> JVM/TI tracing used to be disabled in Client VMs. Don't know
>>> if that's still the case.
>> The jvmti.html file is just copied into the docs bundle later. IMO,
>> the docs bundle should be the same regardless of platform. In
>> practice we only publish the bundle from one build platform anyway.
>>
>> /Erik
>>>
>>> hotspot/makefiles/HotspotCommon.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/gensrc/GenerateSources.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/gensrc/GensrcAdlc.gmk
>>> L98: # NOTE: Windows adlc flags was different in the old
>>> build. Is this really
>>> L99: # correct?
>>> John Rose may know the answer to this historical question.
>>>
>>> hotspot/makefiles/gensrc/GensrcDtrace.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/gensrc/GensrcJvmti.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/ide/CreateVSProject.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/CompileDtracePostJvm.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/CompileDtracePreJvm.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/CompileJvm.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/CompileLibjsig.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/CompileLibraries.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/JvmFeatures.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/JvmMapfile.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/lib/JvmOverrideFiles.gmk
>>> No comments.
>>>
>>> hotspot/makefiles/mapfiles/libjsig/mapfile-vers-solaris
>>> hotspot/makefiles/mapfiles/libjvm_db/mapfile-vers
>>> hotspot/makefiles/mapfiles/libjvm_dtrace/mapfile-vers
>>> No comments on the mapfiles.
>>>
>>> hotspot/makefiles/symbols/symbols-aix
>>> hotspot/makefiles/symbols/symbols-aix-debug
>>> hotspot/makefiles/symbols/symbols-linux
>>> hotspot/makefiles/symbols/symbols-macosx
>>> hotspot/makefiles/symbols/symbols-shared
>>> hotspot/makefiles/symbols/symbols-solaris
>>> hotspot/makefiles/symbols/symbols-solaris-dtrace-compiler1
>>> hotspot/makefiles/symbols/symbols-solaris-dtrace-compiler2
>>> hotspot/makefiles/symbols/symbols-unix
>>> No comments on the symbol files.
>>>
>>>
>>> Thumbs up on this fix; I don't think that anything I noted
>>> above is a show stopper for this changeset.
>>>
>>> Dan
>>>
>>>
>>>>
>>>> /Erik
>>>
>>
>
More information about the hotspot-dev
mailing list