RE: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled
Hello Bernd, 45 return (short) (((x >> 8) & 0xFF) | (x << 8)); Or return (short)((x << 8) | ((x >> 8) & 0xFF)); In the above two lines of code, whichever you choose to write, they both will give the same result. Short will be implicitly converted to int and then the operation will take place. If you write the code as below (to make it consistent with int and long), you will get an error saying “possible lossy conversion from int to short” return ((short) (x << 8) | ((x >> 8) & 0xFF)); or return ((short) (x << 8) | (short)((x >> 8) & 0xFF)); So an explicit cast will be required in both the above cases. Let me know if you would want to see the code changed to “return (short)((x << 8) | ((x >> 8) & 0xFF));” to make it look consistent with the rest of the code. (Since I was not in to/cc, it took me longer to see the mail) -Sharath Ballal From: Bernd Eckenfels [mailto:e...@zusammenkunft.net] Sent: Thursday, November 10, 2016 2:12 PM To: serviceability-dev@openjdk.java.net Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled Hello, I was talking about the code order not the semantics: 45 return (short) (((x >> 8) & 0xFF) | (x << 8)); 54 return ((int)swapShort((short) x) << 16) | (swapShort((short) (x >> 16)) & 0x); 63 return ((long)swapInt((int) x) << 32) | (swapInt((int) (x >> 32)) & 0x); In 45 the Low half is expressed first (and no masking) in 54 and 63 the MSB half is expressed first. I guess casting and masking and order should be the same for all 3? Gruss Bernd -- http://bernd.eckenfels.net On Thu, Nov 10, 2016 at 9:30 AM +0100, "Sharath Ballal" mailto:sharath.bal...@oracle.com"; \nsharath.bal...@oracle.com> wrote: Hello Bernd, For int also its “low 2 bytes | high 2 bytes” and again these two byte pairs are swapped by the swapshort as “lowbyte | highbyte” Similarly for long its “ low 4 bytes | high 4 bytes” and again these pairs are swapped recursively. So for int if the little endian byte order was 3 2 1 0 it is now converted to 0 1 2 3 and similarly for long. -Sharath Ballal From: Bernd Eckenfels [mailto:e...@zusammenkunft.net] Sent: Thursday, November 10, 2016 1:20 PM To: HYPERLINK "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net; Dmitry Samersoff; Sharath Ballal Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled Hello, Is the a reason why swapShort has a " lowbyte | highbyyte" and the other two methods the other way around? I would all write in the natural order of. Igendianess (I.e. Change the first). Gruss Bernd -- http://bernd.eckenfels.net On Thu, Nov 10, 2016 at 8:32 AM +0100, "Sharath Ballal" mailto:sharath.bal...@oracle.com"; \nsharath.bal...@oracle.com> wrote: Thanks Dmitry. I have made the changes in line 54. http://cr.openjdk.java.net/~sballal/7107013/webrev.01/ I didn't change the recursive calls to swap functions because that looks more readable. -Sharath Ballal -Original Message- From: Dmitry Samersoff Sent: Thursday, November 10, 2016 12:46 AM To: Sharath Ballal; HYPERLINK "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled Sharath, Please, add (int) to ll. 54 for better readability. PS: Despite the fact that C2 does a great job eliminating useless code (multiple calls to if (!swap) in this case) it would be nice to use simple, well known arithmetic directly instead of subsequent calls to other swap functions. -Dmitry On 2016-11-09 19:30, Sharath Ballal wrote: > Hello, > > Pls review this small fix > > > > Issue: https://bugs.openjdk.java.net/browse/JDK-7107013 > > Webrev: http://cr.openjdk.java.net/~sballal/7107013/webrev.00/ > > > > > > -Sharath Ballal > > > > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
On 11/10/2016 10:00 AM, David Holmes wrote:
Looks fine to me.
+1
Thanks for fixing!
/Robbin
Thanks,
David
On 10/11/2016 4:56 PM, Ujwal Vangapally wrote:
Thanks for the Review, please find the new webrev incorporating the
review comments.
webrev :
http://cr.openjdk.java.net/~asapre/sponsorships/Ujwal/JDK-8168141/webrev.02/
-Ujwal.
On 11/8/2016 5:18 PM, David Holmes wrote:
Sorry didn't see Staffan's earlier reply :)
David
On 8/11/2016 9:23 PM, David Holmes wrote:
On 8/11/2016 8:44 PM, Robbin Ehn wrote:
Hi Ujwal,
synchronized(li) {
while (li.received < 1) {
li.wait(100);
}
}
I don't see why we need while loop ?
You should always perform a wait() in a loop checking the condition that
is being waited upon. This guards against lost-notifications and also
spurious wakeups.
To me it looks like you could just do:
synchronized(li) {
li.wait();
}
Since either we got notification and received must be bigger than 0.
Or jtreg timed out.
If the notifyAll() happened before you get here then you will wait()
until jtreg does time you out - even though the notification correctly
occurred.
That said, in this particular case doing a timed-wait achieves nothing
other than waking the thread so that it can go back to waiting again.
The "received" value will only change when a notifyAll occurs so there
is no need to poll it (unless aborting due to a timeout as per the
previous version).
Because the loop will never exit, unless the thread is interrupted, this
subsequent code has no affect:
112 if (li.received < 1) {
113 throw new RuntimeException("No notif received!");
David
-
/Robbin ('r'eviewer)
On 11/04/2016 12:03 PM, Ujwal Vangapally wrote:
Please review this small change for the bug below
https://bugs.openjdk.java.net/browse/JDK-8168141
Webrev:
http://cr.openjdk.java.net/~asapre/sponsorships/Ujwal/JDK-8168141/webrev.01/
Thanks,
Ujwal.
Re: RFR: JDK-8168141: javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java: No notif received!
Looks fine to me.
Thanks,
David
On 10/11/2016 4:56 PM, Ujwal Vangapally wrote:
Thanks for the Review, please find the new webrev incorporating the
review comments.
webrev :
http://cr.openjdk.java.net/~asapre/sponsorships/Ujwal/JDK-8168141/webrev.02/
-Ujwal.
On 11/8/2016 5:18 PM, David Holmes wrote:
Sorry didn't see Staffan's earlier reply :)
David
On 8/11/2016 9:23 PM, David Holmes wrote:
On 8/11/2016 8:44 PM, Robbin Ehn wrote:
Hi Ujwal,
synchronized(li) {
while (li.received < 1) {
li.wait(100);
}
}
I don't see why we need while loop ?
You should always perform a wait() in a loop checking the condition that
is being waited upon. This guards against lost-notifications and also
spurious wakeups.
To me it looks like you could just do:
synchronized(li) {
li.wait();
}
Since either we got notification and received must be bigger than 0.
Or jtreg timed out.
If the notifyAll() happened before you get here then you will wait()
until jtreg does time you out - even though the notification correctly
occurred.
That said, in this particular case doing a timed-wait achieves nothing
other than waking the thread so that it can go back to waiting again.
The "received" value will only change when a notifyAll occurs so there
is no need to poll it (unless aborting due to a timeout as per the
previous version).
Because the loop will never exit, unless the thread is interrupted, this
subsequent code has no affect:
112 if (li.received < 1) {
113 throw new RuntimeException("No notif received!");
David
-
/Robbin ('r'eviewer)
On 11/04/2016 12:03 PM, Ujwal Vangapally wrote:
Please review this small change for the bug below
https://bugs.openjdk.java.net/browse/JDK-8168141
Webrev:
http://cr.openjdk.java.net/~asapre/sponsorships/Ujwal/JDK-8168141/webrev.01/
Thanks,
Ujwal.
Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled
Hello, I was talking about the code order not the semantics: 45 return (short) (((x >> 8) & 0xFF) | (x << 8)); 54 return ((int)swapShort((short) x) << 16) | (swapShort((short) (x >> 16)) & 0x); 63 return ((long)swapInt((int) x) << 32) | (swapInt((int) (x >> 32)) & 0x); In 45 the Low half is expressed first (and no masking) in 54 and 63 the MSB half is expressed first. I guess casting and masking and order should be the same for all 3? Gruss Bernd -- http://bernd.eckenfels.net On Thu, Nov 10, 2016 at 9:30 AM +0100, "Sharath Ballal" wrote: Hello Bernd, For int also its “low 2 bytes | high 2 bytes” and again these two byte pairs are swapped by the swapshort as “lowbyte | highbyte” Similarly for long its “ low 4 bytes | high 4 bytes” and again these pairs are swapped recursively. So for int if the little endian byte order was 3 2 1 0 it is now converted to 0 1 2 3 and similarly for long. -Sharath Ballal From: Bernd Eckenfels [mailto:e...@zusammenkunft.net] Sent: Thursday, November 10, 2016 1:20 PM To: serviceability-dev@openjdk.java.net; Dmitry Samersoff; Sharath Ballal Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled Hello, Is the a reason why swapShort has a " lowbyte | highbyyte" and the other two methods the other way around? I would all write in the natural order of. Igendianess (I.e. Change the first). Gruss Bernd -- http://bernd.eckenfels.net On Thu, Nov 10, 2016 at 8:32 AM +0100, "Sharath Ballal" wrote:Thanks Dmitry. I have made the changes in line 54.http://cr.openjdk.java.net/~sballal/7107013/webrev.01/ I didn't change the recursive calls to swap functions because that looks more readable. -Sharath Ballal -Original Message-From: Dmitry Samersoff Sent: Thursday, November 10, 2016 12:46 AMTo: Sharath Ballal; serviceability-...@openjdk.java.netSubject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled Sharath, Please, add (int) to ll. 54 for better readability. PS: Despite the fact that C2 does a great job eliminating useless code (multiple calls to if (!swap) in this case) it would be nice to use simple, well known arithmetic directly instead of subsequent calls to other swap functions. -Dmitry On 2016-11-09 19:30, Sharath Ballal wrote:> Hello,> > Pls review this small fix> > > > Issue: https://bugs.openjdk.java.net/browse/JDK-7107013> > Webrev: http://cr.openjdk.java.net/~sballal/7107013/webrev.00/> > > > > > -Sharath Ballal> > > > > --Dmitry SamersoffOracle Java development team, Saint Petersburg, Russia* I would love to change the world, but they won't give me the sources.
RE: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled
Hello Bernd, For int also its “low 2 bytes | high 2 bytes” and again these two byte pairs are swapped by the swapshort as “lowbyte | highbyte” Similarly for long its “ low 4 bytes | high 4 bytes” and again these pairs are swapped recursively. So for int if the little endian byte order was 3 2 1 0 it is now converted to 0 1 2 3 and similarly for long. -Sharath Ballal From: Bernd Eckenfels [mailto:e...@zusammenkunft.net] Sent: Thursday, November 10, 2016 1:20 PM To: serviceability-dev@openjdk.java.net; Dmitry Samersoff; Sharath Ballal Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled Hello, Is the a reason why swapShort has a " lowbyte | highbyyte" and the other two methods the other way around? I would all write in the natural order of. Igendianess (I.e. Change the first). Gruss Bernd -- http://bernd.eckenfels.net On Thu, Nov 10, 2016 at 8:32 AM +0100, "Sharath Ballal" mailto:sharath.bal...@oracle.com"; \nsharath.bal...@oracle.com> wrote: Thanks Dmitry. I have made the changes in line 54. http://cr.openjdk.java.net/~sballal/7107013/webrev.01/ I didn't change the recursive calls to swap functions because that looks more readable. -Sharath Ballal -Original Message- From: Dmitry Samersoff Sent: Thursday, November 10, 2016 12:46 AM To: Sharath Ballal; HYPERLINK "mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net Subject: Re: RFR: JDK-7107013: sun.jvm.hotspot.runtime.Bytes.swapLong conversion to long mishandled Sharath, Please, add (int) to ll. 54 for better readability. PS: Despite the fact that C2 does a great job eliminating useless code (multiple calls to if (!swap) in this case) it would be nice to use simple, well known arithmetic directly instead of subsequent calls to other swap functions. -Dmitry On 2016-11-09 19:30, Sharath Ballal wrote: > Hello, > > Pls review this small fix > > > > Issue: https://bugs.openjdk.java.net/browse/JDK-7107013 > > Webrev: http://cr.openjdk.java.net/~sballal/7107013/webrev.00/ > > > > > > -Sharath Ballal > > > > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
