Re: [9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi, Mandy and I have agreed in a personal discussion that Mandy will take this issue and target it for a future issue. So I retract this RFR. Thank you, everybody, for the discussions and help! Best regards, Zoltan
Re: [9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi Mandy, On 11/16/2016 07:11 AM, Mandy Chung wrote: On Nov 15, 2016, at 4:41 AM, Zoltán Majó <zoltan.m...@oracle.com> wrote: I think the sentence in webrev.01 (1) "If a registered referenceceases to be strongly reachable itself, then it may never be enqueued." is sufficient. I think we want to make a statement only about what happens when a reference ceases to be strongly reachable (i.e., that in that case we can't make a statement about that reference being enqueued or not). When a reference becomes unreachable (not by examining the source code but the actual state of the VM at runtime), it will never be enqueued. I also like that wording, thank you! The sentence you suggested (2) "A Java virtual machine may implement optimization that could affects when objects become unreachable." adds too much ambiguity as it leaves room for speculation about what "optimization" might be. For example, statement (2) can be possibly (mis-)read as the JVM making references unreachable *earlier* than what a programmer may think -- based on the source code.(That is clearly not the case: References are available at least until the latest program point where they are used, otherwise the program would encounter an error.) It's indeed a JVM "optimization" keep references alive *longer* than it seems to be from the source code. But (1) already encapsulates that without referring to JVM optimizations explicitly. That’s fair. What I’m looking for is something like this [1]: Reachability is not determined by the statements in your source code but by the actual state of the virtual machine at runtime. OK, I see now -- thank you for explaining. Here is a new webrev (webrev.03) that includes what you suggested: http://cr.openjdk.java.net/~zmajo/8169000/webrev.03/ We could go either with this webrev or with the previous one (webrev.02) that changes "will never be enqueued" to "may never be enqueued". http://cr.openjdk.java.net/~zmajo/8169000/webrev.02/ I'm not sure which webrev is the better option. Thank you! Best regards, Zoltan Mandy [1] The Java Programming Language, Fourth Edition, section 17.5.4
Re: [9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi David, On 11/16/2016 03:21 AM, David Holmes wrote: Hi Zoltan, First, I'm okay with latest webrev. thank you. Second, I don't want to confuse things but need to correct one thing ... I think you rather clarified things. On 15/11/2016 10:41 PM, Zoltán Majó wrote: [...] For example, statement (2) can be possibly (mis-)read as the JVM making references unreachable *earlier* than what a programmer may think -- based on the source code.(That is clearly not the case: References are available at least until the latest program point where they are used, otherwise the program would encounter an error.) Actually it can make references unreachable earlier than may be expected. The wording Mandy suggested comes from the JLS itself - see 12.6.1. That is right -- thank you for pointing to the relevant section of the JLS. (Though I agree it isn't the right disclaimer for the current situation - where the interpreter keeps the reference reachable longer than naively expected). Please see my reply to Mandy's email. Thank you! Best regards, Zoltan David - It's indeed a JVM "optimization" keep references alive *longer* than it seems to be from the source code. But (1) already encapsulates that without referring to JVM optimizations explicitly. [...] Making it clear “strongly reachable” is a good suggestion. I don’t see word-smithing is needed in the original sentence; hence my above suggested change only adds the word “strongly” in this sentence. I agree. Here is the updated webrev: http://cr.openjdk.java.net/~zmajo/8169000/webrev.02/ Thank you! Best regards, Zoltan Mandy
Re: [9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi Mandy, thank you for looking at this! Please find more details below. On 11/15/2016 02:51 AM, Mandy Chung wrote: On Nov 14, 2016, at 6:28 AM, Zoltán Majó <zoltan.m...@oracle.com> wrote: Here is the updated webrev with the updated text: http://cr.openjdk.java.net/~zmajo/8169000/webrev.01/ This spec uses “unreachable” to refer to when GC detects as unreachable. I think the current spec is correct. What about this suggested clarification? diff --git a/src/java.base/share/classes/java/lang/ref/package-info.java b/src/java.base/share/classes/java/lang/ref/package-info.java --- a/src/java.base/share/classes/java/lang/ref/package-info.java +++ b/src/java.base/share/classes/java/lang/ref/package-info.java @@ -77,8 +77,12 @@ * references that are registered with it. If a registered reference * becomes unreachable itself, then it will never be enqueued. It is * the responsibility of the program using reference objects to ensure - * that the objects remain reachable for as long as the program is - * interested in their referents. + * that the objects remain strongly reachable for as long as the program + * is interested in their referents. + * + * + * A Java virtual machine may implement optimization that could + * affects when objects become unreachable. * * While some programs will choose to dedicate a thread to * removing reference objects from one or more queues and processing I think the sentence in webrev.01 (1) "If a registered referenceceases to be strongly reachable itself, then it may never be enqueued." is sufficient. I think we want to make a statement only about what happens when a reference ceases to be strongly reachable (i.e., that in that case we can't make a statement about that reference being enqueued or not). The sentence you suggested (2) "A Java virtual machine may implement optimization that could affects when objects become unreachable." adds too much ambiguity as it leaves room for speculation about what "optimization" might be. For example, statement (2) can be possibly (mis-)read as the JVM making references unreachable *earlier* than what a programmer may think -- based on the source code.(That is clearly not the case: References are available at least until the latest program point where they are used, otherwise the program would encounter an error.) It's indeed a JVM "optimization" keep references alive *longer* than it seems to be from the source code. But (1) already encapsulates that without referring to JVM optimizations explicitly. [...] Making it clear “strongly reachable” is a good suggestion. I don’t see word-smithing is needed in the original sentence; hence my above suggested change only adds the word “strongly” in this sentence. I agree. Here is the updated webrev: http://cr.openjdk.java.net/~zmajo/8169000/webrev.02/ Thank you! Best regards, Zoltan Mandy
Re: [9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi David,
On 11/14/2016 11:52 PM, David Holmes wrote:
[...]
I don't think you need the "(i.e ...)". You are cross referencing to
the Reachability section where "strongly reachable" is defined.
I see. OK, I've removed the "i.e.,".
The fewer the changes the better - the key part is to make it clearer
that it "may" never be enqueued, without getting bogged down with why,
or why not.
I agree.
Of course this will need to go through CCC.
Thank you for letting me know. I'll take care of the CCC approval once
we have a changeset that we all agree upon.
Please see the updated webrev in my reply to Mandy.
Best regards,
Zoltan
Thanks,
David
Does that look reasonable to you?
Thank you!
Best regards,
Zoltan
The following modified test shows this situation:
public class WeaklyReachablePhantomReference {
static ReferenceQueue rq = new ReferenceQueue<>();
static WeakReference weakRefRef;
public static void main(final String[] args) throws Exception {
weakRefRef = new WeakReference<>(
new PhantomReference<>(
new Object(),
rq
)
);
// <- here
System.gc();
Reference rmRef = rq.remove(1000);
if (rmRef == null) {
System.out.println("PhantomReference NOT enqueued");
} else {
System.out.println("PhantomReference enqueued");
}
}
}
At "<-- here" the PhantomReference object becomes weakly reachable
while its referent becomes phantom reachable and this is enough for
PhantomReference to not be enqueued.
Regards, Peter
Re: [9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi Peter,
On 11/14/2016 10:59 PM, Peter Levart wrote:
Hi Zoltan,
On 11/14/2016 03:28 PM, Zoltán Majó wrote:
[...]
thank you for the suggestion and for the example program!
Here is the updated webrev with the updated text:
http://cr.openjdk.java.net/~zmajo/8169000/webrev.01/
Does that look reasonable to you?
Yes, I think this is good. Maybe just a nit. This last statement:
"It is the responsibility of the program using reference objects to
ensure that the objects remain strongly reachable for as long as the
program is interested in their referents."
...could be written more nicely like:
"It is the responsibility of the program to ensure that reference
objects remain strongly reachable for as long as it is interested in
their referents."
...or even:
"It is the responsibility of the program to ensure that reference
objects remain strongly reachable for as long as it is interested in
tracking the reachability of their referents."
What do you think?
yes, it sounds better, but probably it's best if we keep this change to
a minimum. So I'd add only the word "strongly" to that sentence.
Please see the updated webrev in my reply to Mandy.
Thank you!
Best regards,
Zoltan
Regards, Peter
Thank you!
Best regards,
Zoltan
The following modified test shows this situation:
public class WeaklyReachablePhantomReference {
static ReferenceQueue rq = new ReferenceQueue<>();
static WeakReference<PhantomReference> weakRefRef;
public static void main(final String[] args) throws Exception {
weakRefRef = new WeakReference<>(
new PhantomReference<>(
new Object(),
rq
)
);
// <- here
System.gc();
Reference rmRef = rq.remove(1000);
if (rmRef == null) {
System.out.println("PhantomReference NOT enqueued");
} else {
System.out.println("PhantomReference enqueued");
}
}
}
At "<-- here" the PhantomReference object becomes weakly reachable
while its referent becomes phantom reachable and this is enough for
PhantomReference to not be enqueued.
Regards, Peter
Re: [9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi Peter,
On 11/11/2016 04:33 PM, Peter Levart wrote:
[...]
I think the wording could be even less specific about "detecting" the
reachability of the reference object. For example:
... If a registered reference becomes unreachable itself, then it
*may* never be enqueued.
In addition, the situations that describe when the reference *may* not
be enqueued could be expanded. For example:
... If a registered reference ceases to be strongly reachable itself,
then it *may* never be enqueued.
thank you for the suggestion and for the example program!
Here is the updated webrev with the updated text:
http://cr.openjdk.java.net/~zmajo/8169000/webrev.01/
Does that look reasonable to you?
Thank you!
Best regards,
Zoltan
The following modified test shows this situation:
public class WeaklyReachablePhantomReference {
static ReferenceQueue rq = new ReferenceQueue<>();
static WeakReference weakRefRef;
public static void main(final String[] args) throws Exception {
weakRefRef = new WeakReference<>(
new PhantomReference<>(
new Object(),
rq
)
);
// <- here
System.gc();
Reference rmRef = rq.remove(1000);
if (rmRef == null) {
System.out.println("PhantomReference NOT enqueued");
} else {
System.out.println("PhantomReference enqueued");
}
}
}
At "<-- here" the PhantomReference object becomes weakly reachable
while its referent becomes phantom reachable and this is enough for
PhantomReference to not be enqueued.
Regards, Peter
[9] RFR (XS): 8169000: Define reference reachability more precisely in java.lang.ref package
Hi, please review the fix for 8169000: https://bugs.openjdk.java.net/browse/JDK-8169000 http://cr.openjdk.java.net/~zmajo/8169000/webrev.00/ The bug was filed because different behavior of interpreted and compiled code in HotSpot was observed (different behavior with respect to phantom references). After discussions with Maurizio C, Alex B, and David H, the best way to address this problem seems to be to update update the documentation of the java.lang.ref to avoid confusion in the future. David's comment in the bug report [1] accurately and concisely summarizes the reasons for the suggested patch. For more details please feel free to look at the comments in the bug report. Thank you! Best regards, Zoltan [1] https://bugs.openjdk.java.net/browse/JDK-8169000?focusedCommentId=14021250=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14021250
Re: [9] RFR(XS): 8137173: @HotSpotIntrinsicCandidate is not Oracle-specific
Hi Mark, On 10/15/2015 11:12 PM, mark.reinh...@oracle.com wrote: 2015/10/1 10:57 -0700, zoltan.m...@oracle.com: On 10/01/2015 05:54 PM, mark.reinh...@oracle.com wrote: I suggest putting this into jdk.internal.vm.annotation, which is also a good place for the ReservedStackAccess annotation envisioned in JEP 270 (http://openjdk.java.net/jeps/270). I filed an RFE, JDK-8138732: "move @HotSpotIntrinsicCandidate to the jdk.internal.vm.annotation package" [1], to track the issue of moving the annotation to a different package. I hope I can take care of it soon. Thank you and best regards, Thanks. While you're at it, could you please rename the annotation to @IntrinsicCandidate? There's no need for the "HotSpot" prefix any more since this annotation will now be in a package that is reserved for VM-specific annotations. (Sorry for this late suggestion; this just came up in a discussion of the @ReservedStackAccess annotation for JEP 270, which is also destined for the jdk.internal.vm.annotation package.) (I've pasted this addendum into JDK-8138732.) thank you for updating the bug description! Best regards, Zoltan - Mark
[9] RFR(XS): 8137173: @HotSpotIntrinsicCandidate is not Oracle-specific
Hi, please review the patch for JDK-8137173. https://bugs.openjdk.java.net/browse/JDK-8137173 Problem: Mark Rheinhold filed this bug and suggested the following: "The doc comment for the @jdk.internal.HotSpotIntrinsicCandidate annotation says that it is "specific to the Oracle Java HotSpot Virtual Machine implementation". That's not true. The annotation is defined in the open-source HotSpot repository, so it is in fact available in all implementations derived from that code, whether from Oracle or from other vendors." Solution: I adopted the solution proposed by Mark. Webrev: http://cr.openjdk.java.net/~zmajo/8137173/webrev.00/ Testing: JPRT run (build + tests). I intend to push this into jdk9/hs-comp/jdk, from there it will eventually propagate to all repositories. Thank you and best regards, Zoltan
Re: [9] RFR(XS): 8137173: @HotSpotIntrinsicCandidate is not Oracle-specific
P.S.: On 10/01/2015 05:31 PM, Zoltán Majó wrote: Problem: Mark Rheinhold filed this bug and suggested the following: I ment Mark Reinhold (w/o the 'h'). Sorry for the mistake. Best regards, Zoltan "The doc comment for the @jdk.internal.HotSpotIntrinsicCandidate annotation says that it is "specific to the Oracle Java HotSpot Virtual Machine implementation". That's not true. The annotation is defined in the open-source HotSpot repository, so it is in fact available in all implementations derived from that code, whether from Oracle or from other vendors." Solution: I adopted the solution proposed by Mark. Webrev: http://cr.openjdk.java.net/~zmajo/8137173/webrev.00/ Testing: JPRT run (build + tests). I intend to push this into jdk9/hs-comp/jdk, from there it will eventually propagate to all repositories. Thank you and best regards, Zoltan
Re: [9] RFR(XS): 8137173: @HotSpotIntrinsicCandidate is not Oracle-specific
Hi Alan, Hi Mark, On 10/01/2015 05:54 PM, mark.reinh...@oracle.com wrote: 2015/10/1 8:41 -0700, alan.bate...@oracle.com: On 01/10/2015 16:31, Zoltán Majó wrote: please review the patch for JDK-8137173. https://bugs.openjdk.java.net/browse/JDK-8137173 ... Webrev: http://cr.openjdk.java.net/~zmajo/8137173/webrev.00/ This looks okay. Is it time to discuss moving the annotation to another package too? thank you for the review! Yes. We don't want to create another sun.misc-like dumping ground with jdk.internal. We should prefer small, descriptive subpackages for things like this so that, when they are (inevitably) exposed via -XaddExports, their exposure can be limited. I suggest putting this into jdk.internal.vm.annotation, which is also a good place for the ReservedStackAccess annotation envisioned in JEP 270 (http://openjdk.java.net/jeps/270). I filed an RFE, JDK-8138732: "move @HotSpotIntrinsicCandidate to the jdk.internal.vm.annotation package" [1], to track the issue of moving the annotation to a different package. I hope I can take care of it soon. Thank you and best regards, Zoltan [1] https://bugs.openjdk.java.net/browse/JDK-8138732 - Mark
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
Hi, I updated the code to also consider intrinsics that were recently added for the following methods: - java.math.BigInteger.implMontgomeryMultiply - java.math.BigInteger.implMontgomerySquare - java.util.zip.CRC32C.updateBytes - java.util.zip.CRC32C.updateDirectByteBuffer In the latest webrev (webrev.08) three files have changed relative to the previous webrev (webrev.07): - hotspot: src/share/vm/classfile/vmSymbols.hpp (merge conflict) - jdk: src/java.base/share/classes/java/util/zip/CRC32.java and src/java.base/share/classes/java/math/BigInteger.java (annotations were added to intrinsified methods) Here is the updated webrev: - hotspot: http://cr.openjdk.java.net/~zmajo/8076112/hotspot/webrev.08/ - jdk: http://cr.openjdk.java.net/~zmajo/8076112/jdk/webrev.08/ All JPRT tests pass. I plan to push the newest webrev (webrev.08) on Friday (July 3) if no other issues come up by then. Thank you and best regards, Zoltan On 07/01/2015 10:54 AM, Zoltán Majó wrote: Hi, thank you, everyone, for the comments/suggestions/feedback! If no other issues come up, I intend to push the latest webrev (webrev.07) on Thursday (July 2). Best regards, Zoltan
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
On 06/29/2015 10:47 PM, John Rose wrote: On Jun 29, 2015, at 10:48 AM, Doug Simon doug.si...@oracle.com mailto:doug.si...@oracle.com wrote: As I understand it, part of this change is to split intrinsification into one method that does the checks that then calls a second method which the VM may intrinsify on the assumption all checks have been performed by the first method. What’s to prevent a direct call to the latter via reflection that by-passes the first method? The answer is simple: We mark the dangerous method private. I assume you are thinking about setAccessible, but if that's allowed there's nothing to prevent people from taking whole system down in a hundred different ways. At that point having a surprising intrinsic is the least of our problems. I understand that writing the checking logic in Java is desirable. Indeed, that is the key compromise here. Coding the required checks in assembly code or compiler IR is (as we all know) a losing battle. You need Maxine/Graal snippets or real Java code to get it right. Thank you, Doug, for bringing up these issues. Thank you, John, for the clarifications. Best regards, Zoltan — John
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
Hi, On 06/29/2015 11:45 AM, Andrew Haley wrote: Hi, On 29/06/15 10:41, Zoltán Majó wrote: On 06/27/2015 10:05 AM, Andrew Haley wrote: On 25/06/15 12:49, Zoltán Majó wrote: Problem: There is need to indicate Java methods that are potentially intrinsified by JVM. It's a great idea but is it a good name? HotSpot is not the only Java VM. Do we expect people from to come along and add J9IntrinsicCandidate, CACAOIntrinsicCandidate, and so on? thank you bringing up this issue. The name HotSpotIntrinsicCandidate resulted from a private discussion with Joe Darcy, Brian Goetz, and John Rose. The reason this name was picked is to make clear that a marked method's interaction with the VM (specifically with the HotSpot VM implementation) needs special attention. OK, cool. So has any thought been given to the other VMs? Do you expect that, say, J9 will use the HotSpotIntrinsicCandidate annotattion, or do you expect we will have similar annotations for each VM which uses OpenJDK libraries? Or is the need for this annotation totally HotSpot-specific? the need for this annotation resulted from the way HotSpot handles intrinsics. Here are the two main reasons: (1) Intrinsics in the HotSpot VM omit some checks (typically null checks and array bounds checks) that are instead performed in the JDK code. If HotSpot intrinsic code is changed, the matching JDK code must be changed as well (and vice versa). Otherwise we might run into correctness problems (e.g., the HotSpot intrinsic and the JDK method have different semantics) and/or performance problems (HotSpot suddenly not intrinsify a method because, e.g., the method's signature has changed and HotSpot's intrinsic list was not updated accordingly). Annotating intrinsified methods makes it less likely that a mismatch between a JDK method and its HotSpot-level intrinsic counterpart can be introduced. (2) With the newly added CheckIntrinsic flag, HotSpot verifies if all annotated methods are backed by intrinsics at the VM level and that all intrinsics are marked appropriately in the JDK. Other VM implementations will most likely intrinsify a different set of methods. So, if those methods were marked with the same annotation as HotSpot is looking for, it would be difficult for HotSpot to check the match between intrinsics and the JDK code they replace (Reason 2 from above). Also, if a JDK method is updated for which VM_A but not VM_B defines and intrinsic, only VM A's intrinsic code must be updated to match the JDK code, so it is maybe better to mark clearly which VM implementation intrinsifies an annotated method. So, the current design would require introducing a similar annotation for every VM that decided to implement what we just proposed for HotSpot with the current changeset. Thank you and best regards, Zoltan Andrew.
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
Hi James, thank you for your feedback! I've implemented the changes you suggested, here is the updated webrev: - top: http://cr.openjdk.java.net/~zmajo/8076112/top/webrev.07/ - jdk: http://cr.openjdk.java.net/~zmajo/8076112/jdk/webrev.07/ - hotspot: http://cr.openjdk.java.net/~zmajo/8076112/hotspot/webrev.07/ Best regards, Zoltan On 06/26/2015 07:23 PM, james cheng wrote: Hi Zoltán, I am not a reviewer. Just saw some typos in code comments: 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/ in SHA*.java, 'implCompressImpl', as parameter the method - hotspot: http://cr.openjdk.java.net/~zmajo/8076112/hotspot/webrev.06/ in vmSymbols.hpp.html, 649 // ... e.g.,, 650651[ one of the closing parentheses seems superfluous] 673 // ... approriate ... Thanks, -James
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
On 06/28/2015 09:21 PM, Alan Bateman wrote: On 26/06/2015 16:43, Zoltán Majó wrote: 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, Thanks, looks like my comments are addressed so looks good to me. Thank you, Alan! Best regards, Zoltan -Alan
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
Hi Andrew, On 06/27/2015 10:05 AM, Andrew Haley wrote: On 25/06/15 12:49, Zoltán Majó wrote: Problem: There is need to indicate Java methods that are potentially intrinsified by JVM. It's a great idea but is it a good name? HotSpot is not the only Java VM. Do we expect people from to come along and add J9IntrinsicCandidate, CACAOIntrinsicCandidate, and so on? thank you bringing up this issue. The name HotSpotIntrinsicCandidate resulted from a private discussion with Joe Darcy, Brian Goetz, and John Rose. The reason this name was picked is to make clear that a marked method's interaction with the VM (specifically with the HotSpot VM implementation) needs special attention. Best regards, Zoltan Andrew.
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
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
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
Hi Joe, On 06/25/2015 07:30 PM, joe darcy wrote: Hi Zoltán, The changes to the wrapper classes (java.lang.Integer, java.lang.Double, ...), the Math and StrictMath libraries, and the new annotation type look fine. thank you for the review! Best regards, Zoltán Thanks, -Joe On 6/25/2015 4:49 AM, 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/ Testing: - JPRT run with the 'hotspot' testset, all tests pass; - all JTREG hotspot tests, all tests pass that pass with a VM version that does not include the patch; all tests were run with -XX:+CheckIntrinsics. Thank you and best regards, Zoltan
[9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
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/ Testing: - JPRT run with the 'hotspot' testset, all tests pass; - all JTREG hotspot tests, all tests pass that pass with a VM version that does not include the patch; all tests were run with -XX:+CheckIntrinsics. Thank you and best regards, Zoltan
