[8u] Request for enhancement backport approval for 8210352: [hotspot] --with-extra-cxx-flags should not lower optimization

Tue Sep 11 09:07:17 UTC 2018

On Mon, 2018-09-10 at 19:04 +0100, Andrew Hughes wrote:
> On Thu, 6 Sep 2018 at 10:46, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> > 
> > On Wed, 2018-09-05 at 20:06 +0100, Andrew Hughes wrote:
> > > On Wed, 5 Sep 2018 at 09:55, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > I've been asked to proceed with the enhancement request process[1] for
> > > > this proposed JDK 8u fix.
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210352
> > > > 
> > > > Currently JDK 8 and JDK 9+ behave differently in terms of hotspot
> > > > libjvm optimization flags. In JDK 8 they can be overridden by
> > > > --with-extra-c{,xx}-flags configure options. In JDK 9+ this cannot be
> > > > done as libjvm optimization flags come after any extra C flags from
> > > > configure. This enhancement request for JDK 8 asks for the build of the
> > > > JVM to behave the same as JDK 9+.
> > > > 
> > > > The rationale why we'd wanted such behaviour is as follows. In Fedora
> > > > distribution wide config exists which sets C flags. Hooks are in place
> > > > so that all natives get compiled with these C flags. One goal is to
> > > > ensure all distro libraries and programs are being optimized at at
> > > > least a certain threshold, say >= -O2. This includes OpenJDK.
> > > > 
> > > > While there is a way to fix the build system to compile each and every
> > > > library with some optimization level the risk is that there might not
> > > > be a one-size-fits-all downstream distribution consumers. The easier
> > > > and more flexible case is to allow for this via extra C flags which can
> > > > be passed to the build. This is currently possible.
> > > > 
> > > > The exception to this rule should be libjvm. Optimization levels for
> > > > the JVM have been thoroughly tested and to some extent been lowered via
> > > > a by-object overrides. Example case is
> > > > sharedRuntimeTrans.o and sharedRuntimeTrig.o which have -O0
> > > > settings[2]. The point is that for the JVM optimization flags have been
> > > > thoroughly crafted to a point where they are today. Letting the user
> > > > override this to some other value, like -O2, via a simple configure
> > > > option seems dangerous. Currently in JDK 8 it's possible to override
> > > > JVM optimization flags via --with-extra-c{,xx}-flags configure option.
> > > > 
> > > > OK to move ahead with this enhancement for JDK 8u?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > [1] http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023064.html
> > > > [2] http://hg.openjdk.java.net/jdk8u/jdk8u-dev/hotspot/file/80ee2541504e/make/linux/makefiles/amd64.make#l25
> > > > 
> > > 
> > > I tend to agree with David's comment:
> > > 
> > > "If I set an extra cflag I expect it to be applied to all compilations
> > > - cross-compilation would be
> > > broken if that were not the case. I don't see why -On should be
> > > treated any differently." [0]
> > 
> > This seems to be a comment on an old webrev. It does not apply to the
> > updated one here which adds the OPT flags *after* user CFLAGS for the
> > JVM:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210352/webrev.02/
> > 
> > As to "the flags should get applied to all compilations" issue: I'd
> > think that's not an issue for the updated webrev, is it? They get
> > applied, but OPT flags come before any JVM set flags.
> 
> David's comment is on an earlier version, yes, but it still applies.
> Specifically, 'I don't see why -On should be treated any differently.'.
> By adding -On flags after user -On flags and overriding them, it is
> being treated differently from earlier flags, which user cflags would
> be able to override.

Fair enough.

> > > and we've encountered the same issue in Gentoo in the past [1].
> > > 
> > > Your example seems a little disingenuous, especially given you're
> > > proposing to remove
> > > those -O0 examples in another bug [2].
> > 
> > Fair point. I can come up with others. The hotspot build seems to be
> > sprinkled with optimization overrides:
> > 
> > http://hg.openjdk.java.net/jdk8u/jdk8u-dev/hotspot/file/80ee2541504e/make/solaris/makefiles/sparc.make#l34
> > http://hg.openjdk.java.net/jdk8u/jdk8u-dev/hotspot/file/80ee2541504e/make/linux/makefiles/gcc.make#l259
> > http://hg.openjdk.java.net/jdk8u/jdk8u-dev/hotspot/file/80ee2541504e/make/linux/makefiles/fastdebug.make#l35
> > 
> > They could be summarized as work-arounds for GCC bugs. If user
> > supplied -O flags override all these, then they might get broken
> > builds. I question that's a good approach...
> 
> Then that's the choice of the user. Given that two of these are for
> old compilers, and the other for an unsupported architecture, I think
> the risk is pretty low. Even lower when you consider these failures
> are probably based on compiling with -O3.
> 
> > 
> > > Given that the code is
> > > generally compiled with -O3,
> > > the more common effect of overriding the optimisation is going to be
> > > to lower it.
> > 
> > Since the hotspot build already has slowdebug as a build option I'm not
> > sure. Do we really have data that somebody wants release builds with
> > -O2 (over -O3) with the risk of getting some mis-compiled objects in
> > the JVM?
> 
> slowdebug means not just no optimisation, but turning on a raft of assertion
> code paths. It's a much bigger step than just toning down the optimisation.
> 
> The bug that brought you to this webrev in the first place was because -O2
> was being ignored in your builds.

Somewhat. My issue would be solved if the OpenJDK build *ignored* any
optimization flags passed via extra C flags *and* had default opt flags
for all hotspot libs.

In summary, I have:

 * The build gets --with-extra-cflags="-O2" --with-extra-cxxflags="-
   O2". Note that it might be -O1 or something else. The point is,
   those C flags come from a global package (redhat-rpm-config). The
   OpenJDK distro build just passes them along via that switch.
 * Assume I added patches so that libsaproc and libjsig get a default
   opt level.
 * JDK libraries seem to add optimization flags *after* any extra C
   flags. Examples: fdlibm, libawt_xawt, libjava, etc.

The resulting build would optimize libjvm, libsaproc, libjsig at level
-O2 (or whatever the value was from redhat-rpm-config's C flags)
unconditionally. Adding patches for default libjsig and libsaproc
optimization won't help. They'd get the opt value from the extra C
flags anyway. JDK libraries would be compiled at the optimization
levels library writers deemed appropriate. -O3 for libjava, -02 for
libawt_xawt, -O0 for fdlibm, etc.

One solution to my problem would be to:
 * Add default opt levels to otherwise not optimized libs (libjsig,
   libsaproc).
 * Be sure to filter -O flags being passed to the OpenJDK build
   downstream.

Such a solution would still keep this inconsistency between hotspot and
core libraries in terms of opt-flags handling, though.

My proposal was to make the behaviour consistent across the OpenJDK
build (core libs and hotspot). By letting default opt flags override
extra c flags, I'd get the issue of libjsig and libsaproc not being
optimized solved too, since they don't specify a default. That seemed
like something upstream OpenJDK would be interested in having and here
we are :)

Of course, For consistency's sake, I could go the other way and change
core libs. Then again, it wouldn't help with my issue, nor would it
make it any more consistent with JDK 9+ builds.

> Unless I misunderstand it, this entire
> fix seems to be aimed at allowing you to pass -O2 and then have it bumped
> to -O3 by the build in most cases. Wouldn't it make more sense to fix the
> few errant cases that lack optimisation in release builds rather than altering
> everyone's build to suit your use case?

Playing devils advocate: Everyone's build is already altered to suit my
use case when we'd only look at core libraries in JDK 8 :)

> <snip as it's basically just the same argument>
> 
> > 
> > It seems we disagree. It looks like the better choice to me. After all,
> > user defined global optimization for the JVM might produce wrong code.
> > Doesn't that make it too easy for somebody to shoot themselves in the
> > foot?
> 
> I think the fundamental issue here is whether you assume the user to
> be a fool or not. In my experience, it's far more likely that someone
> will pass arguments to the build for whatever reason, and then get annoyed
> when the build blindly ignores them, than they will override optimisations
> and then complain when it breaks things. In the latter case, they've made
> the choice and get to keep the pieces. There's very much a control
> argument here rather than a technical one.
> 
> How likely is it that someone will pass their own optimisation in the first
> place?

Not likely. I'd argue the choices made in core libs and JDK 9+ have its
merrit.

> The original problem here feels contrived, because you essentially
> want to pass your own optimisation flags, but then not use them in the
> majority of cases.
> 
> > 
> > > The right solution here seems to me to be to add optimisations to
> > > those areas that
> > > lack it, in appropriate build modes. It seems likely to me that jsig
> > > and saproc were
> > > overlooked because of the use of separate makefiles in the old HotSpot
> > > build more
> > > than anything else (as seems to have been the case with execstack in 8187045).
> > 
> > Sure. It can be done. In that case we'd have to decide on some
> > appropriate level of optimization for those. Perhaps -O2?
> 
> I think that's a much better solution. -O2 seems a good compromise for
> older builds, where no optimisation has been the norm, but perhaps they
> could use the system-wide -O3 on 12.

As explained above, these patches are somewhat orthogonal to my issue
at hand. That being said, I can add them too.

> Have you done much testing with these compiled with optimisation? The main
> thing we need to ascertain is whether their exclusion was a mistake or
> a deliberate choice because it breaks these tools.

I'm putting my money on the former, but I'll do some more testing.

Incidentally my proposal would be leaner towards finding this out
through experience. People can change this via extra c-flags and see
whether it causes issues for them, and only them: It gives them a
downstream tunable.

Thanks,
Severin

> > 
> > > Are they optimised in 9? Or is there any evidence of breakage caused by
> > > using optimisation there?
> > 
> > They don't seem to be on 12:
> > 
> > $ grep -C3 SetupJdkLibrary make/lib/Lib-jdk.hotspot.agent.gmk
> > 
> > ################################################################################
> > 
> > $(eval $(call SetupJdkLibrary, BUILD_LIBSA, \
> >     NAME := saproc, \
> >     OPTIMIZATION := NONE, \
> >     DISABLED_WARNINGS_microsoft := 4267, \
> > 
> > I haven't checked on 9. I don't think there is evidence of breakage
> > caused by optimization as nobody likely uses optimization for those.
> > That doesn't mean it might not break, though :-)
> 
> Indeed :-)
> 
> So what we really need to be looking at here is these particular cases
> and whether they can be optimised, rather than trying to alter the whole
> build so that these cases are inadvertently optimised when someone
> passes in -O2.
> 
> And yes, I should have said 9+ rather than 9. I doubt there are that
> many build changes in this area in the last year.
> 
> > 
> > Thanks,
> > Severin
> > 
> > > [0] http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023060.html
> > > [1] https://bugs.gentoo.org/356991
> > > [2] https://bugs.openjdk.java.net/browse/JDK-8210425
> 
> 