Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation
I have a couple nit-picky comments: 1. The change to src/jdk.jlink/share/classes/module-info.java is unrelated to jpackage and should be reverted (there is only a white-space change and a copyright date change to that file) 2. The following files have whitespace errors that will cause jcheck to fail: src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java:326: Trailing whitespace src/jdk.jpackage/share/classes/jdk/jpackage/internal/CLIHelp.java:58: Tab character src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java:217: Trailing whitespace src/jdk.jpackage/share/classes/jdk/jpackage/internal/ValidOptions.java:55: Trailing whitespace 3. I recommend to replace the wild-card imports with explicit imports, for example: src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxDebBundler.java (java.util.* and java.io.*) (I think the wild-card static import is fine, just not the import every class from a package) I'll try to remember to note these as I go through the review. This one could be done as a follow-up bug rather than doing it prior to integration if you prefer. -- Kevin
Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation
A couple of questions / observations :-
1) setlocale
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/linux/native/jpackageapplauncher/launcher.cpp.html
52 int main(int argc, char *argv[]) {
53 int result = 1;
54 setlocale(LC_ALL, "en_US.utf8");
Why is this setlocale() call there ?
What does this mean for a user whose desktop is (say) German, or French, or
Japanese ?
When the Java app is launched from this environment is it inheriting this US
locale ? I hope not.
We have the same on Mac :-
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/native/jpackageapplauncher/main.m.html
and windows :-
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/native/jpackageapplauncher/WinLauncher.cpp.html
64 ::setlocale(LC_ALL, "en_US.utf8");
2) C++ files containing C
src/jdk.jpackage/windows/native/libjpackage/WindowsRegistry.cpp
src/jdk.jpackage/windows/native/libjpackage/jpackage.cpp
src/jdk.jpackage/windows/native/libwixhelper/libwixhelper.cpp
have their entire contents wrapped in
36 #ifdef __cplusplus
37 extern "C" {
38 #endif
159 #ifdef __cplusplus
160 }
161 #endif
wouldn't it be better to put them in .c files ?
-phil.
Re: RFR: JDK-8222913: Add Jib support for VERSION_EXTRA*
Erik - Looks good to me as well. Tim On 04/24/19 16:27, Mikael Vidstedt wrote: Wow, so proactive of you. Ship it! Cheers, Mikael On Apr 24, 2019, at 4:00 PM, Erik Joelsson wrote: JDK-8207849 added support for extra version numbers in the java version string. Now we need to use at least one more at Oracle so need to add the corresponding support to jib-profiles.js. Bug: https://bugs.openjdk.java.net/browse/JDK-8222913 Webrev: http://cr.openjdk.java.net/~erikj/8222913/webrev.01/ /Erik
Re: [8u] RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation
Hi Matthias,
Thanks for the review!
On Tue, 2019-04-30 at 12:52 +, Baesken, Matthias wrote:
> Hi Severin, looks okay to me (not a reviewer however).
> In our proprietary JVM8 we hadset (on
> Linux)BUILD_LIBFDLIBM_OPTIMIZATION :=
> HIGH and FDLIBM_CFLAGS += -ffp-contract=offfor a long time
> and did not observe any issues .
Good to know. We do the same in Fedora, fwiw.
> (but we always build with gcc 4.8.x so cannot tell much about lower
> gcc versions ).
>
> But I really wonder - wouldn’t it be better in the long run to go
> for a min. gcc versions >= 4.6 (or even 4.8) in OpenJDK8 ?
>
> Maybe some people from the distros could comment on this .
Perhaps, but it's hard to effectively figure out what that minimal
version should be.
Note that this change for the alternative of -ffp-contract=off was
introduced to JDK 8 by SAP. See:
https://bugs.openjdk.java.net/browse/JDK-8172053
Reading that bug it appears that minimum gcc is version 4.3? Either
way, this patch will do the right thing(tm) for both as far as I could
tell. It would use -mno-fused-madd -fno-strict-aliasing over --ffp-
contract=off.
My focus for this backport was to not break existing behaviour.
Supporting -ffp-contract=off only would also break our upstream build
machinery, as for them we build on RHEL 6 with gcc 4.4.7.
>From our point of view JDK 8 minimal GCC would be 4.4.7 :)
Thanks,
Severin
>
> Best regards, Matthias
>
>
> > -Original Message-
> > From: ppc-aix-port-dev
> > On
> > Behalf Of Severin Gehwolf
> > Sent: Dienstag, 30. April 2019 13:38
> > To: jdk8u-dev ; build-dev > d...@openjdk.java.net>
> > Cc: ppc-aix-port-dev
> > Subject: [8u] RFR: 8210416: [linux] Poor StrictMath performance due
> > to non-
> > optimized compilation
> >
> > Hi,
> >
> > Could I please get a review for this 8u backport related to fdlibm
> > optimization on Linux? The JDK 12 patch doesn't apply as-is as the
> > JDK
> > 8 build system is drastically different from JDK 11+.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8210416
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8210416/jdk8/02/
> >
> > The main differences to the original fix are: a) optimization level
> > and
> > b) additional hook for older GCC. This backport keeps the
> > optimization
> > level at -O3 (HIGH) over -O2 (LOW) for JDK 8u as this would
> > otherwise
> > regress Linux ppc64{,le} which currently use -O3. As the current
> > code
> > has the implicit assumption of ppc64 being compiled on older GCCs
> > too
> > (JDK-8172053), this backport maintains compatibility in this
> > regard. If
> > -ffp-contract=off is not available, a machine specific set of flags
> > is
> > being used if the compiler supports them (-mno-fused-madd -fno-
> > strict-
> > aliasing).
> >
> > For older GCCs (< 4.6) specific machine flags are being used. That
> > is,
> > for ppc64{,le} and x86{,_64}. ppc64{,le} machine specific flags
> > have
> > already been determined (See JDK-8172053). x86_64 and x86 have the
> > same
> > machine specific flags available, so I've used them there too[1].
> >
> > Testing: build/test on gcc 8.x Linux x86_64. build/test on gcc
> > 4.4.7
> > x86_64/ppc64. Manual inspection of build logs for fdlibm files
> > (e.g.
> > w_asin.c).
> >
> > Thoughts?
> >
> > Thanks,
> > Severin
> >
> > [1]
> > https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/i386-and-x86_002d64-
> > Options.html#i386-and-x86_002d64-Options
> >
> >
> >
> >
RE: [8u] RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation
Hi Severin, looks okay to me (not a reviewer however).
In our proprietary JVM8 we hadset (on Linux)
BUILD_LIBFDLIBM_OPTIMIZATION := HIGH and FDLIBM_CFLAGS +=
-ffp-contract=offfor a long time and did not observe any issues .
(but we always build with gcc 4.8.x so cannot tell much about lower gcc
versions ).
But I really wonder - wouldn’t it be better in the long run to go for a min.
gcc versions >= 4.6 (or even 4.8) in OpenJDK8 ?
Maybe some people from the distros could comment on this .
Best regards, Matthias
> -Original Message-
> From: ppc-aix-port-dev On
> Behalf Of Severin Gehwolf
> Sent: Dienstag, 30. April 2019 13:38
> To: jdk8u-dev ; build-dev d...@openjdk.java.net>
> Cc: ppc-aix-port-dev
> Subject: [8u] RFR: 8210416: [linux] Poor StrictMath performance due to non-
> optimized compilation
>
> Hi,
>
> Could I please get a review for this 8u backport related to fdlibm
> optimization on Linux? The JDK 12 patch doesn't apply as-is as the JDK
> 8 build system is drastically different from JDK 11+.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210416
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> 8210416/jdk8/02/
>
> The main differences to the original fix are: a) optimization level and
> b) additional hook for older GCC. This backport keeps the optimization
> level at -O3 (HIGH) over -O2 (LOW) for JDK 8u as this would otherwise
> regress Linux ppc64{,le} which currently use -O3. As the current code
> has the implicit assumption of ppc64 being compiled on older GCCs too
> (JDK-8172053), this backport maintains compatibility in this regard. If
> -ffp-contract=off is not available, a machine specific set of flags is
> being used if the compiler supports them (-mno-fused-madd -fno-strict-
> aliasing).
>
> For older GCCs (< 4.6) specific machine flags are being used. That is,
> for ppc64{,le} and x86{,_64}. ppc64{,le} machine specific flags have
> already been determined (See JDK-8172053). x86_64 and x86 have the same
> machine specific flags available, so I've used them there too[1].
>
> Testing: build/test on gcc 8.x Linux x86_64. build/test on gcc 4.4.7
> x86_64/ppc64. Manual inspection of build logs for fdlibm files (e.g.
> w_asin.c).
>
> Thoughts?
>
> Thanks,
> Severin
>
> [1] https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/i386-and-x86_002d64-
> Options.html#i386-and-x86_002d64-Options
>
>
>
>
[8u] RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation
Hi,
Could I please get a review for this 8u backport related to fdlibm
optimization on Linux? The JDK 12 patch doesn't apply as-is as the JDK
8 build system is drastically different from JDK 11+.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210416
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/jdk8/02/
The main differences to the original fix are: a) optimization level and
b) additional hook for older GCC. This backport keeps the optimization
level at -O3 (HIGH) over -O2 (LOW) for JDK 8u as this would otherwise
regress Linux ppc64{,le} which currently use -O3. As the current code
has the implicit assumption of ppc64 being compiled on older GCCs too
(JDK-8172053), this backport maintains compatibility in this regard. If
-ffp-contract=off is not available, a machine specific set of flags is
being used if the compiler supports them (-mno-fused-madd -fno-strict-
aliasing).
For older GCCs (< 4.6) specific machine flags are being used. That is,
for ppc64{,le} and x86{,_64}. ppc64{,le} machine specific flags have
already been determined (See JDK-8172053). x86_64 and x86 have the same
machine specific flags available, so I've used them there too[1].
Testing: build/test on gcc 8.x Linux x86_64. build/test on gcc 4.4.7
x86_64/ppc64. Manual inspection of build logs for fdlibm files (e.g.
w_asin.c).
Thoughts?
Thanks,
Severin
[1]
https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/i386-and-x86_002d64-Options.html#i386-and-x86_002d64-Options
[11u] RFR 8210782: Upgrade HarfBuzz to the latest 2.3.1
Hi,
please help reviewing the backport of JDK-8210782: Upgrade HarfBuzz to the
latest 2.3.1.
This has been backported to 11.0.4-oracle already. I took the large change down
to 11u-dev. It applies quite nicely, apart from a little issue in
make/lib/Awt2dLibraries.gmk:
--- Awt2dLibraries.gmk
+++ Awt2dLibraries.gmk
@@ -613,8 +614,7 @@
type-limits missing-field-initializers implicit-fallthrough \
strict-aliasing undef unused-function, \
DISABLED_WARNINGS_CXX_gcc := reorder delete-non-virtual-dtor
strict-overflow \
-maybe-uninitialized \
-missing-attributes class-memaccess, \
+maybe-uninitialized class-memaccess, \
DISABLED_WARNINGS_clang := unused-value incompatible-pointer-types \
tautological-constant-out-of-range-compare int-to-pointer-cast \
sign-compare undef missing-field-initializers, \
The original change would remove the disabling of the "missing-attributes"
warnings, but since this warning type is not disabled in jdk11u-dev currently,
I would just skip that diff.
With that change, most platforms did build fine, except for Solaris (where we
use Oracle Studio 12u4 in jdk11u-dev vs Oracle Studio 12u6 in jdk/jdk) and AIX
(xlc12 vs. xlc16).
To keep support for Oracle Studio 12u4 for Solaris, I had to remove the warning
flag "refmemnoconstr_aggr" from line 622 (as opposed to the original change).
Seems that it is not yet supported in OS12u4.
Furthermore, a code tweak had to be done (Thanks, Matthias, for your help here):
---
a/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-subset-cff-common.hh
Fri Mar 01 16:59:19 2019 -0800
+++
b/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-subset-cff-common.hh
Mon Apr 29 16:26:41 2019 +0200
@@ -280,6 +280,10 @@
{
str_buff_t
bool drop_hints;
+
+ // Solaris: OS12u4 complains about "A class with a reference member lacks a
user-defined constructor"
+ // so provide the constructor
+ flatten_param_t(str_buff_t& sbt, bool dh) : flatStr(sbt), drop_hints(dh) {}
};
template
@@ -305,7 +309,9 @@
return false;
cs_interpreter_t interp;
interp.env.init (str, acc, fd);
- flatten_param_t param = { flat_charstrings[i], drop_hints };
+ // Solaris: OS12u4 does not like the C++11 style init
+ // flatten_param_t param = { flat_charstrings[i], drop_hints };
+ flatten_param_t param(flat_charstrings[i], drop_hints);
if (unlikely (!interp.interpret (param)))
return false;
}
For AIX, this tweak had to be added (credit goes to Matthias as well):
diff -r 2b3dbedfbfb9
src/java.desktop/share/native/libfontmanager/harfbuzz/hb-null.hh
--- a/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-null.hh Fri Mar
01 16:59:19 2019 -0800
+++ b/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-null.hh Mon Apr
29 16:26:41 2019 +0200
@@ -83,7 +83,9 @@
template
struct _hb_assign
-{ static inline void value (T , const V v) { o = v; } };
+// add cast to please AIX xlc12.1
+//{ static inline void value (T , const V v) { o = v; } };
+{ static inline void value (T , const V v) { o = (T&) v; } };
template
struct _hb_assign >
{ static inline void value (T , const V v) { o.set (v); } };
Bug: https://bugs.openjdk.java.net/browse/JDK-8210782
Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/7c11a7cc7c1d
Review discussion for jdk/jdk:
https://mail.openjdk.java.net/pipermail/2d-dev/2019-March/009914.html
New Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8210782.jdk11u/
Thanks & Best regards
Christoph
