Review Request for MVT incubator module
maurizio.cimadamore at oracle.com
Fri Sep 15 12:04:43 UTC 2017
Hi Mandy, this looks like a solid piece of work, thanks!
* I applied all patches and run all relevant tests. Hotspot and JDK are
fine, but langtools is getting some new failures - more specifically, I
get two of those:
The failure in (1) seems to be related to some changes you have made to
the CP entries indices. Those changes make the test fail on my setup -
but that has to do with the way I run tests - I always use the
-javacoption:-g option, which sometimes catches interesting issues (yes,
I'm paranoid). TThat said, if the test passes or fails depending on
configuration, it should be @ignored, at least for now - I've never been
too much of a fan of these tests that compare javap output against some
golden files, they are a maintenance nightmare.
The failure in (2) is genuine, and had to do with the fact that the test
is trying to complete a module (jdk.incubator.mvt) too early, which will
cause an assertion error when running the test. I've discussed a similar
problem wityh Jan while ago, and I came up with the workaround which I
have now applied to your case (see attached patch). With the patch the
issue in the test is fixes.
* separately, as discussed privately, generally I'm not a big fan of
making high level changes such as turning incubator warnings into notes
in an experimental branch. If we feel that's the way to go, that change
should go into jdk10 and then flow back here. But I understand that in
this case, to make existing tests work in the face of the unsuppressible
warnings, you have to tweak tests to use the --should-stop internal
option - so that means you have to change tests now, and then back once
the enhancement comes from jdk10. So perhaps, given the warning 2 note
change is a one liner, we might be able to live with it.
* I also tried several test scenarios in which custom annotations called
ValueCapableClass were used instead of the 'official' one in the
incubator module, but I have not been able to come up with any real
issues - my concern was that the analysis that the VM applied to the
annotation is mostly textual, but there seem to be a couple of saving
factors: (i) if the EnableMVT flag is not used, the VM will not even
attempt to look at annotations, (ii) when the flag is used, the
incubator module (which is added automatically when the flag is set)
will automatically supplant any classes coming from the classpath - so
the right VCC will be used. In short, I have not been able to make the
VM 'trip up' on some maliciously written fake VCC anno. Which is good.
On 15/09/17 07:02, mandy chung wrote:
> This patch proposes to define an incubator module  for MVT, named
> `jdk.incubator.mvt`. Specifically this
> movesjvm.internal.value.ValueCapableClass and
> jdk.experimental.value.ValueType APIto a new jdk.incubator.mvt package.
> -XX:+EnableMVT continues to be the runtime option to enable MVT. I
> have changed the runtime to add jdk.incubator.mvt when MVT is
> enabled. For compile-time, compiling @VCC classes would require to
> run javac with `--add-modules jdk.incubator.mvt` as incubator modules
> are not resolved by default.
> A couple points worth mentioning:
> java.base can't reference @VCC and ValueType class statically since
> they are now in a different module. In addition, @VCC can only be
> loaded after startup asVM startup can only load classes in java.base.
> jdk.incubator.mvtis defined by the platform loader (as we want it to
> be deprivileged for security reason) and so Class.forName to find
> classes in jdk.incubator.module should use the right loader.
> jdk.incubator.mvt.ValueType is basically a wrapper around the internal
> ValueTypeHolder class that is what java.base references in the
> implementation. MethodHandleBuilder is kept as internal API for now.
> I leave it as a future exercise if we decide to expose that API in the
> incubator module.
> The tests are updated with @modules jdk.incubator.mvt or add access to
> I temporarily downgrade the javac incubating module warning to a note
> so that -Werror has no effect. We will need to revisit this warning in
> JDK 10.
>  http://openjdk.java.net/jeps/11
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 1170 bytes
Desc: not available
More information about the valhalla-dev