Review Request for MVT incubator module

[Maurizio Cimadamore]

Hi Mandy, this looks like a solid piece of work, thanks!

Few comments:

* 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:

1) langtools/test/tools/javac/valhalla/values/CheckNoInvokeDirect.java
2) langtools/test/tools/javac/processing/model/TestSymtabItems.java

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.

Cheers
Maurizio


On 15/09/17 07:02, mandy chung wrote:
> Webrev:
>    http://cr.openjdk.java.net/~mchung/valhalla/webrevs/incubator/
>
> This patch proposes to define an incubator module [1] 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 
> jdk.experimental
>
> 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.
>
> thanks
> Mandy
> [1] http://openjdk.java.net/jeps/11

-------------- next part --------------
A non-text attachment was scrubbed...
Name: langtools-test.patch
Type: text/x-patch
Size: 1170 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/valhalla-dev/attachments/20170915/07c7de47/langtools-test.patch>