Re: RFR: JDK-8152666: The new Hotspot Build System

Thu, 07 Apr 2016 06:54:52 -0700 

> 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

Reply via email to