Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8179084

HotSpot VM fails to start when AggressiveHeap is set

    Details

    • Subcomponent:
      gc
    • Introduced In Build:
      b131
    • Introduced In Version:
      9
    • Resolved In Build:
      b168
    • CPU:
      generic
    • OS:
      generic
    • Verification:
      Verified

      Backports

        Description

        FULL PRODUCT VERSION :
        java version "1.8.0_131"
        Java(TM) SE Runtime Environment (build 1.8.0_131-b11)
        Java HotSpot(TM) 64-Bit Server VM (build 25.131-b11, mixed mode)

        FULL OS VERSION :
        Mac OS X Sierra 10.12.4
        Darwin ID-PC14001.local 16.5.0 Darwin Kernel Version 16.5.0: Fri Mar 3 16:52:33 PST 2017; root:xnu-3789.51.2~3/RELEASE_X86_64 x86_64

        A DESCRIPTION OF THE PROBLEM :
        When specifying -XX:+AggressiveHeap as JVM argument the VM now aborts with an error message . This used to work perfectly fine in all previous versions of the JRE/JDK

        THE PROBLEM WAS REPRODUCIBLE WITH -Xint FLAG: Yes

        THE PROBLEM WAS REPRODUCIBLE WITH -server FLAG: Yes

        REGRESSION. Last worked in version 8u121

        STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
        In the simplest form even this command line aborts
        "java -XX:+AggressiveHeap -version" with error message
        "The Parallel GC can not be combined with -XX:ParallelGCThreads=0"

        In previous versions this simple cmd line would print the version. But any other combination doesn't work either.

        Additionally trying to use PrintFlagsFinal to check the actual value still causes the error.
        Many products ship with the setting in some config file out of the box (as do our own when set to production level during install)

        PrintFlagsFInal confirms that it was using 8 threads on my machine prior to u131.

        EXPECTED VERSUS ACTUAL BEHAVIOR :
        That the virtual machine starts up with AggressiveHeap enabled without error
        ERROR MESSAGES/STACK TRACES THAT OCCUR :
        "The Parallel GC can not be combined with -XX:ParallelGCThreads=0"

        REPRODUCIBILITY :
        This bug can be reproduced always.

        ---------- BEGIN SOURCE ----------
        java -XX:+AggressiveHeap -version
        ---------- END SOURCE ----------

        CUSTOMER SUBMITTED WORKAROUND :
        You can work arround this by explicitly specifying a value for ParallelGCthreads only if that argument is earlier in the argument list than AggressiveHeap.

        But having to figure out why certain services/java based apps no longer start and having to edit the config files in the right location and order is annoying at best.

          Issue Links

            Activity

            Hide
            fmatte Fairoz Matte added a comment - - edited
            This is a regression introduced in 8u131 b06/ 9ea b131
            ==
            jdk/8u131/fcs/b11/binaries/linux-x64/bin/java -XX:+AggressiveHeap -version
            The Parallel GC can not be combined with -XX:ParallelGCThreads=0
            ==

            8u121 - Pass
            8u131 - Fail
            9 ea b130 - Pass
            9 ea b131 - fail
            9 ea b166 - Fail
            ==

            Suspecting this could be introduced by JDK-8147910
            Show
            fmatte Fairoz Matte added a comment - - edited This is a regression introduced in 8u131 b06/ 9ea b131 == jdk/8u131/fcs/b11/binaries/linux-x64/bin/java -XX:+AggressiveHeap -version The Parallel GC can not be combined with -XX:ParallelGCThreads=0 == 8u121 - Pass 8u131 - Fail 9 ea b130 - Pass 9 ea b131 - fail 9 ea b166 - Fail == Suspecting this could be introduced by JDK-8147910
            Hide
            dholmes David Holmes added a comment -
            Yes this is caused by JDK-8147910. The AggressiveHeap option is processed before we initialize the value:

             // Parse arguments
              jint parse_result = Arguments::parse(args); <= AggressiveHeap processed in here
              if (parse_result != JNI_OK) return parse_result;

              os::init_before_ergo(); <= initial processor count initilaized here.

            Need to re-examine the initialization paths to see where we can move this to. Not sure why it isn't done in os::init().
            Show
            dholmes David Holmes added a comment - Yes this is caused by JDK-8147910 . The AggressiveHeap option is processed before we initialize the value:  // Parse arguments   jint parse_result = Arguments::parse(args); <= AggressiveHeap processed in here   if (parse_result != JNI_OK) return parse_result;   os::init_before_ergo(); <= initial processor count initilaized here. Need to re-examine the initialization paths to see where we can move this to. Not sure why it isn't done in os::init().
            Hide
            dholmes David Holmes added a comment - - edited
            Calling sequence:

            os::initial_active_processor_count()
            => Abstract_VM_Version::nof_parallel_worker_threads()
            ===> Abstract_VM_Version::calc_parallel_worker_threads
            =====> Abstract_VM_Version::parallel_worker_threads()
            =======> Arguments::set_parnew_gc_flags
            =========> Arguments::set_cms_and_parnew_gc_flags()
            ===========> Arguments::set_gc_specific_flags()
            =========> Arguments::set_gc_specific_flags()
            =======> Arguments::set_parallel_gc_flags()
            =========> Arguments::set_gc_specific_flags()
            =======> Arguments::set_g1_gc_flags()
            =========> Arguments::set_gc_specific_flags()
            =======> Arguments::parse_each_vm_init_arg() // JDK 9 factors code into set_aggressive_heap_flags() rather than directly inline

            All except the last end at set_gc_specific_flags():
            => ArgumentsExt::set_gc_specific_flags()
            ===> Arguments::apply_ergo()

            So normally the initial_active_processor_count is only used in this context after Arguments::apply_ergo(), hence os::init_before_ergo() was a suitable location to do initialization.

            For Arguments::parse_each_vm_init_arg()
            => Arguments::parse_vm_init_args()
            ===> Arguments::parse()
            => Arguments::parse_options_environment_variable()
            ===> Arguments::parse_java_options_environment_variable()
            ===> Arguments::parse_java_tool_options_environment_variable()
            =====> Arguments::parse_vm_init_args()
            ======> Arguments::parse()

            Will now check initialization possibilities for os::initialize_initial_active_processor_count().
            Show
            dholmes David Holmes added a comment - - edited Calling sequence: os::initial_active_processor_count() => Abstract_VM_Version::nof_parallel_worker_threads() ===> Abstract_VM_Version::calc_parallel_worker_threads =====> Abstract_VM_Version::parallel_worker_threads() =======> Arguments::set_parnew_gc_flags =========> Arguments::set_cms_and_parnew_gc_flags() ===========> Arguments::set_gc_specific_flags() =========> Arguments::set_gc_specific_flags() =======> Arguments::set_parallel_gc_flags() =========> Arguments::set_gc_specific_flags() =======> Arguments::set_g1_gc_flags() =========> Arguments::set_gc_specific_flags() =======> Arguments::parse_each_vm_init_arg() // JDK 9 factors code into set_aggressive_heap_flags() rather than directly inline All except the last end at set_gc_specific_flags(): => ArgumentsExt::set_gc_specific_flags() ===> Arguments::apply_ergo() So normally the initial_active_processor_count is only used in this context after Arguments::apply_ergo(), hence os::init_before_ergo() was a suitable location to do initialization. For Arguments::parse_each_vm_init_arg() => Arguments::parse_vm_init_args() ===> Arguments::parse() => Arguments::parse_options_environment_variable() ===> Arguments::parse_java_options_environment_variable() ===> Arguments::parse_java_tool_options_environment_variable() =====> Arguments::parse_vm_init_args() ======> Arguments::parse() Will now check initialization possibilities for os::initialize_initial_active_processor_count().
            Hide
            dholmes David Holmes added a comment -
            os::initialize_initial_active_processor_count() calls os::active_processor_count() to do the initialization.

            Analysis of the per-OS implementations show the following VM initialization dependencies:
            - AIX: None (uses sysconf)
            - BSD: None (already initialized during os::init()
            - Linux: None (uses sched_getaffinity)
            - Solaris: None (uses sysconf and pset_bind)
            - Windows: None (uses GetProcessAffinityMask)

            So it is perfectly safe to move os::initialize_initial_active_processor_count() into os::init() (specific placement is needed on BSD).

            The downside of this is that os::init() is platform specific so we have to change all 5 implementations. Alternatively we could call os::initialize_initial_active_processor_count() directly in create_vm immediately after os::init().
            Show
            dholmes David Holmes added a comment - os::initialize_initial_active_processor_count() calls os::active_processor_count() to do the initialization. Analysis of the per-OS implementations show the following VM initialization dependencies: - AIX: None (uses sysconf) - BSD: None (already initialized during os::init() - Linux: None (uses sched_getaffinity) - Solaris: None (uses sysconf and pset_bind) - Windows: None (uses GetProcessAffinityMask) So it is perfectly safe to move os::initialize_initial_active_processor_count() into os::init() (specific placement is needed on BSD). The downside of this is that os::init() is platform specific so we have to change all 5 implementations. Alternatively we could call os::initialize_initial_active_processor_count() directly in create_vm immediately after os::init().
            Hide
            dholmes David Holmes added a comment - - edited
            The code actually sets ParallelGCThreads twice when AggressiveHeap is used:

            // Arguments::parse_each_vm_init_arg()

                  // Enable parallel GC and adaptive generation sizing
                  FLAG_SET_CMDLINE(bool, UseParallelGC, true);
                  FLAG_SET_DEFAULT(ParallelGCThreads,
                                   Abstract_VM_Version::parallel_worker_threads()); <= will be zero at this point and is cached

            then later:

            // Arguments::set_gc_specific_flags()
             if (UseParallelGC || UseParallelOldGC) {
                set_parallel_gc_flags();

            which in turn does:

              FLAG_SET_DEFAULT(ParallelGCThreads,
                               Abstract_VM_Version::parallel_worker_threads()); <= Safe to call here but zero already cached.
              if (ParallelGCThreads == 0) {
                jio_fprintf(defaultStream::error_stream(),
                    "The Parallel GC can not be combined with -XX:ParallelGCThreads=0\n");
                vm_exit(1);
              }

            so it seems to me that the simplest fix here is to delete the first:

                  FLAG_SET_DEFAULT(ParallelGCThreads,
                                   Abstract_VM_Version::parallel_worker_threads());

            as we will set it later anyway.

            Show
            dholmes David Holmes added a comment - - edited The code actually sets ParallelGCThreads twice when AggressiveHeap is used: // Arguments::parse_each_vm_init_arg()       // Enable parallel GC and adaptive generation sizing       FLAG_SET_CMDLINE(bool, UseParallelGC, true);       FLAG_SET_DEFAULT(ParallelGCThreads,                        Abstract_VM_Version::parallel_worker_threads()); <= will be zero at this point and is cached then later: // Arguments::set_gc_specific_flags()  if (UseParallelGC || UseParallelOldGC) {     set_parallel_gc_flags(); which in turn does:   FLAG_SET_DEFAULT(ParallelGCThreads,                    Abstract_VM_Version::parallel_worker_threads()); <= Safe to call here but zero already cached.   if (ParallelGCThreads == 0) {     jio_fprintf(defaultStream::error_stream(),         "The Parallel GC can not be combined with -XX:ParallelGCThreads=0\n");     vm_exit(1);   } so it seems to me that the simplest fix here is to delete the first:       FLAG_SET_DEFAULT(ParallelGCThreads,                        Abstract_VM_Version::parallel_worker_threads()); as we will set it later anyway.
            Hide
            kbarrett Kim Barrett added a comment -
            There don't seem to be any tests at all for -XX:+AggressiveHeap. Also, the documentation for it seems pretty thin.

            I think [~dholmes] suggestion of removing the FLAG_SET_DEFAULT(ParallelGCThreads, ...) form from set_aggressive_heap_flags() is okay for JDK 9 and 8u. In the longer term, we might want to do something a little more principled.

            -XX:+AggressiveHeap is handled in a rather strange way. It is not handled as a normal flag option. (-XX:-AggressiveHeap isn't even recognized.) Instead, while running down the list of CLAs, we call set_aggressive_heap_flags as soon as that option is seen. So the problem at hand can't even be worked around by adding -XX:ParallelGCThreads=<n> to the end of the options; it must preceed the AggressiveHeap option.

            set_aggressive_heap_flags sets a number of options. Some of that looks plausible. But where the value is obtained from something not directly and obviously available at that point, there may be problems. The too early attempt to get the active processor count is one such. I think there might be some similar problems in the heap and new space sizing.
            Show
            kbarrett Kim Barrett added a comment - There don't seem to be any tests at all for -XX:+AggressiveHeap. Also, the documentation for it seems pretty thin. I think [~dholmes] suggestion of removing the FLAG_SET_DEFAULT(ParallelGCThreads, ...) form from set_aggressive_heap_flags() is okay for JDK 9 and 8u. In the longer term, we might want to do something a little more principled. -XX:+AggressiveHeap is handled in a rather strange way. It is not handled as a normal flag option. (-XX:-AggressiveHeap isn't even recognized.) Instead, while running down the list of CLAs, we call set_aggressive_heap_flags as soon as that option is seen. So the problem at hand can't even be worked around by adding -XX:ParallelGCThreads=<n> to the end of the options; it must preceed the AggressiveHeap option. set_aggressive_heap_flags sets a number of options. Some of that looks plausible. But where the value is obtained from something not directly and obviously available at that point, there may be problems. The too early attempt to get the active processor count is one such. I think there might be some similar problems in the heap and new space sizing.
            Hide
            kbarrett Kim Barrett added a comment - - edited
            Fix Request

            Without this fix we have a regression of a failure to start java when using a not uncommon
            command line option.

            The fix is low risk. The removed defaulting of ParallelGCThreads is
            covered by a later defaulting of that option to the same value.

            The proposed change also includes a test to verify simple use of the
            option doesn't crash and has some of the expected effects.

            Webrev of the fix is here:
            http://cr.openjdk.java.net/~kbarrett/8179084/hotspot.00/
            Show
            kbarrett Kim Barrett added a comment - - edited Fix Request Without this fix we have a regression of a failure to start java when using a not uncommon command line option. The fix is low risk. The removed defaulting of ParallelGCThreads is covered by a later defaulting of that option to the same value. The proposed change also includes a test to verify simple use of the option doesn't crash and has some of the expected effects. Webrev of the fix is here: http://cr.openjdk.java.net/~kbarrett/8179084/hotspot.00/
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/jdk9/dev/hotspot/rev/f1cca489e9c6
            User: kbarrett
            Date: 2017-04-27 03:32:08 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/dev/hotspot/rev/f1cca489e9c6 User: kbarrett Date: 2017-04-27 03:32:08 +0000
            Hide
            wyandi Winston Yandi added a comment -
            It is a critical pre approval to push the fix into the mainline repo.
            Show
            wyandi Winston Yandi added a comment - It is a critical pre approval to push the fix into the mainline repo.
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/f1cca489e9c6
            User: lana
            Date: 2017-05-03 19:45:36 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/f1cca489e9c6 User: lana Date: 2017-05-03 19:45:36 +0000
            Hide
            lmesnik Leonid Mesnik added a comment -
            Regression test exists.
            Show
            lmesnik Leonid Mesnik added a comment - Regression test exists.

              People

              • Assignee:
                kbarrett Kim Barrett
                Reporter:
                webbuggrp Webbug Group
              • Votes:
                0 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: