Hi Thomas, >> looks good to me - however there is another (pre-existing) bug: the >> shift in that code should be a logical shift, not an arithmetic >> shift. >> >> I.e. ">>" instead of ">>>". >> >> I will run the patch through testing and report back in a few hours. >> Should be okay. > > is good. Do you want to fix the issue with the shift operator too > here, or use another CR?
Thanks! If the use of ">>>" is the bug, I want to fix it in new bug ticket. I do not think the use of ">>>" is not a bug. I g1BiasedArray.hpp, G1BiasedMappedArray::get_by_address() uses ">>" operator to calculate biased_index: http://hg.openjdk.java.net/jdk/hs/file/ee513596f3ee/src/hotspot/share/gc/g1/g1BiasedArray.hpp#l134 idx_t is defined as size_t. So it is calculated as unsigned value. In JLS 15.19, ">>>" is for unsigned. If we use ">>", it might remain MSB in some cases. https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-15.19 Thus I think this is not a bug. Thanks, Yasumasa 2018-01-31 0:51 GMT+09:00 Thomas Schatzl <[email protected]>: > Hi all, > > On Tue, 2018-01-30 at 11:06 +0100, Thomas Schatzl wrote: >> Hi, >> >> On Tue, 2018-01-30 at 07:00 +1000, David Holmes wrote: >> > Added in hotspot-gc-dev. Although this is in the SA it is about the >> > SA interaction with G1 and so likely needs someone familiar with G1 >> > to review it. >> > >> > David >> > >> > On 28/01/2018 10:41 PM, Yasumasa Suenaga wrote: >> > > PING: Could you review it? >> > > >> > > > > http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/ >> > > >> > > This webrev has been reviewed by Jini. >> > > I need a Reviewer and sponsor. >> >> looks good to me - however there is another (pre-existing) bug: the >> shift in that code should be a logical shift, not an arithmetic >> shift. >> >> I.e. ">>" instead of ">>>". >> >> I will run the patch through testing and report back in a few hours. >> Should be okay. > > is good. Do you want to fix the issue with the shift operator too > here, or use another CR? > > Thanks, > Thomas >
