Hi Alan,
thank you for the review! Please see my answers/comments below.
On 06/26/2015 10:39 AM, Alan Bateman wrote:
On 25/06/2015 12:49, Zoltán Majó wrote:
Hi,
please review the patch for JDK-8076112.
Bug: https://bugs.openjdk.java.net/browse/JDK-8076112
Problem: There is need to indicate Java methods that are potentially
intrinsified by JVM.
Solution: Mark intrinsified methods with the
jdk.internal.HotSpotIntrinsicCandidate annotation. Add checks that
are omitted by VM-level intrinsics to the library code. Add a new
diagnostic flag, CheckIntrinsics. If CheckIntrinsics is enabled, the
VM performs the following checks when a class C is loaded:
- all intrinsics defined by the VM for class C are present in the
loaded class file and are marked;
- an intrinsic is defined by the VM for all marked methods of C.
If a mismatch is detected, the following is done:
- a fastdebug VM prints a warning and then exits;
- a product VM prints a warning and unmarked are not intrinsified.
Webrev:
- top: http://cr.openjdk.java.net/~zmajo/8076112/top/webrev.05/
- jdk: http://cr.openjdk.java.net/~zmajo/8076112/jdk/webrev.05/
- hotspot: http://cr.openjdk.java.net/~zmajo/8076112/hotspot/webrev.05/
I skimmed through the webrev and don't see any significant issues.
One thing to double check is that you are are keeping things locally
consistent. One example is the convention in many areas to use implFoo
rather than fooImpl. Also in some areas then native methods then
you'll see foo0 called from foo. I'm just mentioning it because it
requires coordination to rename these methods.
I changed the naming of most intrinsified methods from fooImpl to
implFoo. In four cases the wrapper of the intrinsified method is already
called implFoo. So I decided to use the convention for native methods in
those cases and renamed the intrinsic to implFoo0. The four methods are:
sun/security/provider/SHA.implCompress0
sun/security/provider/SHA2.implCompress0
sun/security/provider/SHA5.implCompress0
sun/security/provider/implCompressMultiBlock0
You might also want to do a quick pass over to ensure other
consistency. I see a few places where 8 spec indent is used, that
could be just tabs of course.
I updated the indentation as well.
Here is the updated webrev:
- top: http://cr.openjdk.java.net/~zmajo/8076112/top/webrev.06/
- jdk: http://cr.openjdk.java.net/~zmajo/8076112/jdk/webrev.06/
- hotspot: http://cr.openjdk.java.net/~zmajo/8076112/hotspot/webrev.06/
Thank you and best regards,
Zoltan
-Alan
