Review-Request: Fix of JDK-8034775 neglects to account for non-JIT VMs

[Severin Gehwolf]

Hi,

On Tue, 2014-04-29 at 12:29 -0700, Vladimir Kozlov wrote:
> Hi Albert,
> 
> Based on resent mail from Mark we need to publish webrev on our cr. 
> server. Severin sent v2 version and I asked if it is final. Albert if it 
> is final, please, prepare webrev and send official RFR in format we use.

Is there anything I can do to make this easier for you next time?

Thanks,
Severin

> On 4/29/14 11:47 AM, Albert wrote:
> > Iris, thanks for the clarification.
> >
> > Severin, I will push your changes if they are reviewed.
> >
> > Thanks,
> > Albert
> >
> >
> > On 04/29/2014 06:59 PM, Iris Clark wrote:
> >> Hi, Severin.
> >>
> >>> I work for Red Hat and I believe it has signed the OCA as a company.
> >>> Not sure if this makes me a contributor.
> >> You're a Contributor.
> >>
> >> Red Hat is on the Signatories List [1].
> >>
> >> Thanks,
> >> iris
> >>
> >> [1]: http://www.oracle.com/technetwork/community/oca-486395.html
> >>
> >> -----Original Message-----
> >> From: Severin Gehwolf [mailto:sgehwolf at redhat.com]
> >> Sent: Tuesday, April 29, 2014 2:31 AM
> >> To: hotspot-dev at openjdk.java.net
> >> Subject: Re: Review-Request: Fix of JDK-8034775 neglects to account
> >> for non-JIT VMs
> >>
> >> Hi Albert,
> >>
> >> On Tue, 2014-04-29 at 10:36 +0200, Albert wrote:
> >>> Hi,
> >>>
> >>> sorry, it was my change that introduced the bug. The change looks good
> >>> to me as well.
> >> Thanks for looking at the patch!
> >>
> >>> However, I am not a reviewer, so I think a second review would be good.
> >> OK.
> >>
> >>> Or is the change
> >>> simple enough that 1 review is fine? I am not sure.
> >>>
> >>> Are you a contributor? ( http://openjdk.java.net/contribute/ ) If yes,
> >>> I will push your changes as soon as I have an OK.
> >> I work for Red Hat and I believe it has signed the OCA as a company.
> >> Not sure if this makes me a contributor.
> >>
> >> Cheers,
> >> Severin
> >>
> >>> On 04/28/2014 08:13 PM, Vladimir Kozlov wrote:
> >>>> Hi Severin,
> >>>>
> >>>> Your fix looks reasonable.
> >>>>
> >>>> Sorry for breaking your build. I assigned the bug to Albert. He will
> >>>> sponsor your changes.
> >>>>
> >>>> Thanks,
> >>>> Vladimir
> >>>>
> >>>> On 4/28/14 4:42 AM, Severin Gehwolf wrote:
> >>>>> Hi,
> >>>>>
> >>>>> Bug: JI-9011998 (I don't seem to be able to create JDK bugs)
> >>>>> Webrev: http://jerboaa.fedorapeople.org/bugs/openjdk/JI-9011998/v1/
> >>>>>
> >>>>> The fix for JDK-8034775 introduced a start-up check requiring the
> >>>>> number of compiler threads to be >= 1, which does not make sense
> >>>>> for non-JIT VMs such as the zero JVM variant. This causes zero JVMs
> >>>>> to fail initialization with:
> >>>>>
> >>>>> CICompilerCount of 0 is invalid; must be at least 1
> >>>>> Error: Could not create the Java Virtual Machine.
> >>>>> Error: A fatal exception has occurred. Program will exit.
> >>>>>
> >>>>> This is caused by a wrong start-up check in
> >>>>> src/share/vm/runtime/arguments.cpp where a minimal value of 1 is
> >>>>> required no matter the JVM variant.
> >>>>>
> >>>>> The proposed fix uses the defined CI_COMPILER_COUNT pre-processor
> >>>>> constant over a static 1 to pass to verify_min_value(). Since
> >>>>> CI_COMPILER_COUNT is going to be defined differently for JVM
> >>>>> variants it will make the lower water mark check correct for all
> >>>>> JVM variants.
> >>>>>
> >>>>> There was an error in defining CI_COMPILER_COUNT as well. On line
> >>>>> 196 in src/share/vm/runtime/globals.hpp CI_COMPILER_COUNT is
> >>>>> defined to be 0 (since COMPILER1, COMPILER2 and SHARK are not
> >>>>> defined for a Zero build.
> >>>>> Then on line 201 in src/share/vm/runtime/globals.hpp the "else"
> >>>>> branch of ifdef COMPILER2 is entered and the earlier definition of
> >>>>> CI_COMPILER_COUNT (with value 0) overridden to 1.
> >>>>>
> >>>>> I've amended test/compiler/startup/NumCompilerThreadsCheck.java so
> >>>>> as to verify that the lower water mark for Zero JVMs is 0.
> >>>>>
> >>>>> Thoughts?
> >>>>>
> >>>>> Cheers,
> >>>>> Severin
> >>>>>
> >>
> >>
> >