Hello Magnus,
This is certainly a nice improvement. It looks good to me. I have some
comments on implementation details, but nothing serious enough to
require a new webrev.
Instead of using "echo $foo | sed 's/,/ /g'", since we know we run in
bash, could just use ${foo//,/ }. (jvm-features.m4:82)
The $AWK expressions are just copied from the previous implementation,
so they are probably working ok. I would still recommend using $NAWK to
increase likelihood of them really working the same across platforms.
jvm-features.m4:370,372-373: wrong indentation
/Erik
On 2020-02-19 04:05, Magnus Ihse Bursie wrote:
The JVM feature handling in configure needs a complete overhaul. The
current logic is hard to understand and to make changes to. The method
of enabling features with --with-jvm-features="feature list" means
that it is not possible to incrementally add features without throwing
away settings done earlier on the command line.
With this patch, the most noticeable effect for normal users is the
addition of a group of configure arguments, on the pattern
--enable-jvm-feature-<FEATURE>. So, instead of doing e.g.
--with-jvm-features="zgc,-dtrace", you can now do
--enable-jvm-feature-zgc --disable-jvm-feature-dtrace. The major
benefit from this is that it is possible to build up a command line in
steps, where a later step enables or disables a feature, without
throwing away the settings made earlier (which was what happened if
two --with-jvm-features= options were given).
Arguably, this is the way that JVM features should have been
implemented all along. There were ever only two reasons for the
--with-jvm-features argument list, neither of them particularly good:
It allows for simple selection of multiple features (e.g. for the
custom variant), and it avoided the complexity in programmatically
generating autoconf options in m4.
I have now bit the bullet, and wrangled m4 into doing this. The old
way of --with-jvm-features="<list>" is of course still supported, but
I think for the most part, the new style is recommended. Some
features, e.g. cds, had their own options (--enable-cds) which were
weirdly translated into features. These options are now defined as
aliases (of e.g. --enable-jvm-feature-cds), and I intend to keep them
as such.
Under the hood, much more has changed. There is no longer the
schizophrenic split between handling stuff the "old" way or the "new"
way using features. All features are handled the same, period.
Furthermore, the logic has been cleared up considerably.
First of all, I check if a feature is possible to build on your
platform. For instance, CDS is not available on AIX, or dtrace
requires proper tooling. If that is the case, it is considered
unavailable, and it is an error to try to enable it.
This check is also done on a per-variant basis. Some features are not
available on all variants. For instance, the "zero" feature is only
available on the "zero" variant.
Secondly, not all available features should be enabled by default --
even if most should be. Therefore, I keep lists of "filtered" features
(one for the platform and one per variant). The default for a variant
is then calculated as all available features, minus all that are
filtered out.
If a user has explicitly enabled a feature, it is added to the list
(as long as it is available). If a user has disabled a features, it is
removed from the list.
This separation makes it clear if a feature is not built by default
because it is not supported, or because it is not recommended (like
link-time-opt). This distinction was unclear, to say the least, in the
old code.
For platforms that are outside my expertise (like zero and aix), I've
done my best to convert the old behavior to this model, but I'd
appreciate if someone knowledgeable about the code can verify. Zero,
for instance, has a long list of features classified as unavailable,
but I'm not sure that this is correct.
Hopefully, the new code base should make it much easier to understand
what requirements a certain feature has, and why (if not) it is not
built. Also, I hope that adding a new feature in the future would be
far easier, by just following the model.
Bug: https://bugs.openjdk.java.net/browse/JDK-8239450
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8239450-overhaul-JVM-features/webrev.01
/Magnus
