Quantcast

RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
Please review this change to BitMap::get_next_zero_offset_inline to
remove its use of left_n_bits, thereby avoiding
https://bugs.openjdk.java.net/browse/JDK-8178348.

CR:
https://bugs.openjdk.java.net/browse/JDK-8178352

Webrev:
http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.00/

Testing:
JPRT, with the test from
https://bugs.openjdk.java.net/browse/JDK-8169039
included in the tested repository.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Stefan Karlsson
Hi Kim,

On 2017-04-10 00:00, Kim Barrett wrote:
> Please review this change to BitMap::get_next_zero_offset_inline to
> remove its use of left_n_bits, thereby avoiding
> https://bugs.openjdk.java.net/browse/JDK-8178348.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8178352
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.00/


The bug fix looks good.

You did a couple of stylistic changes that now make the code inconsistent:

-    for (; res & 1; res_offset++) {
-      res = res >> 1;

+    for (; (res & 1) == 0; ++res_offset) {
+      res >>= 1;


But the surrounding code uses this format:
  271     for (; !(res & 1); res_offset++) {
  272       res = res >> 1;
  273     }

I think these changes should be kept out of this patch.

Thanks,
StefanK

>
> Testing:
> JPRT, with the test from
> https://bugs.openjdk.java.net/browse/JDK-8169039
> included in the tested repository.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
> On Apr 10, 2017, at 3:44 AM, Stefan Karlsson <[hidden email]> wrote:
>
> Hi Kim,
>
> On 2017-04-10 00:00, Kim Barrett wrote:
>> Please review this change to BitMap::get_next_zero_offset_inline to
>> remove its use of left_n_bits, thereby avoiding
>> https://bugs.openjdk.java.net/browse/JDK-8178348.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8178352
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.00/
>
>
> The bug fix looks good.

Thanks.

> You did a couple of stylistic changes that now make the code inconsistent:
>
> -    for (; res & 1; res_offset++) {
> -      res = res >> 1;
>
> +    for (; (res & 1) == 0; ++res_offset) {
> +      res >>= 1;
>
>
> But the surrounding code uses this format:
> 271     for (; !(res & 1); res_offset++) {
> 272       res = res >> 1;
> 273     }
>
> I think these changes should be kept out of this patch.

Post-decrement in for loops still makes me twitch, even though it doesn’t matter
in this code base.  But okay.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
> On Apr 10, 2017, at 12:18 PM, Kim Barrett <[hidden email]> wrote:
>> You did a couple of stylistic changes that now make the code inconsistent:
>>
>> […]
>> I think these changes should be kept out of this patch.
>
> Post-decrement in for loops still makes me twitch, even though it doesn’t matter
> in this code base.  But okay.

Updated webrev:
http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.01/

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Stefan Karlsson
On 10/04/17 18:33, Kim Barrett wrote:

>> On Apr 10, 2017, at 12:18 PM, Kim Barrett <[hidden email]> wrote:
>>> You did a couple of stylistic changes that now make the code inconsistent:
>>>
>>> […]
>>> I think these changes should be kept out of this patch.
>> Post-decrement in for loops still makes me twitch, even though it doesn’t matter
>> in this code base.  But okay.
> Updated webrev:
> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.01/
>
To be consistent with the other code it in that file (res & 1) == 0
should be !(res & 1). Maybe create a small, trivial cleanup RFE to
change the following in the BitMap files: * !(res & 1) => (res & 1) == 0
* res = res >> 1 => res >>= 1 I'll promise to do a quick review ;) StefanK

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
> On Apr 10, 2017, at 12:48 PM, Stefan Karlsson <[hidden email]> wrote:
>
> On 10/04/17 18:33, Kim Barrett wrote:
>>> On Apr 10, 2017, at 12:18 PM, Kim Barrett <[hidden email]>
>>>  wrote:
>>>
>>>> You did a couple of stylistic changes that now make the code inconsistent:
>>>>
>>>> […]
>>>> I think these changes should be kept out of this patch.
>>>>
>>> Post-decrement in for loops still makes me twitch, even though it doesn’t matter
>>> in this code base.  But okay.
>>>
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.01/
>>
>>
>>
> To be consistent with the other code it in that file (res & 1) == 0 should be !(res & 1).

That would be contrary to hotspot style guide; I think that should take precedence.

> Maybe create a small, trivial cleanup RFE to change the following in the BitMap files:
> * !(res & 1) => (res & 1) == 0
> * res = res >> 1 => res >>= 1

I can do something like that, but would prefer it as a followup.

There are other similar syntactic / style guide cleanups, such as consistent use of
helpers like bit_in_word, word_index, and bit_index.

Having unit tests make these kinds of changes easier.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
> On Apr 10, 2017, at 2:21 PM, Kim Barrett <[hidden email]> wrote:
>
>> On Apr 10, 2017, at 12:48 PM, Stefan Karlsson <[hidden email]> wrote:
>> To be consistent with the other code it in that file (res & 1) == 0 should be !(res & 1).
>
> That would be contrary to hotspot style guide; I think that should take precedence.

Stefan and I discussed this privately, and I’ve agreed to go with the locally consistent style here,
even though it’s contrary to the hotspot style guide.  Part of my decision process is related to
the next part of this reply.
 
>> Maybe create a small, trivial cleanup RFE to change the following in the BitMap files:
>> * !(res & 1) => (res & 1) == 0
>> * res = res >> 1 => res >>= 1
>
> I can do something like that, but would prefer it as a followup.
>
> There are other similar syntactic / style guide cleanups, such as consistent use of
> helpers like bit_in_word, word_index, and bit_index.

On further consideration, I don’t think we should do that. Instead, we should deal with the fact
that we have 3 functions of significant size and complexity, which differ in small ways, some of
which are intentional and some of which are not.  We should instead have one parameterized
implementation.

I’d forgotten that I actually have code that does that.  I’d put it aside because I had more pressing
matters to deal with and that code needed more work, especially testing.  That was actually the
driver for 8169039, which is how we got here…


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
> On Apr 11, 2017, at 1:45 PM, Kim Barrett <[hidden email]> wrote:
>
>> On Apr 10, 2017, at 2:21 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On Apr 10, 2017, at 12:48 PM, Stefan Karlsson <[hidden email]> wrote:
>>> To be consistent with the other code it in that file (res & 1) == 0 should be !(res & 1).
>>
>> That would be contrary to hotspot style guide; I think that should take precedence.
>
> Stefan and I discussed this privately, and I’ve agreed to go with the locally consistent style here,
> even though it’s contrary to the hotspot style guide.  Part of my decision process is related to
> the next part of this reply.

Here’s the updated webrev:
http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.02/

Need a second reviewer, in addition to waiting for Stefan.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Stefan Karlsson
On 2017-04-19 08:40, Kim Barrett wrote:

>> On Apr 11, 2017, at 1:45 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On Apr 10, 2017, at 2:21 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>>> On Apr 10, 2017, at 12:48 PM, Stefan Karlsson <[hidden email]> wrote:
>>>> To be consistent with the other code it in that file (res & 1) == 0 should be !(res & 1).
>>>
>>> That would be contrary to hotspot style guide; I think that should take precedence.
>>
>> Stefan and I discussed this privately, and I’ve agreed to go with the locally consistent style here,
>> even though it’s contrary to the hotspot style guide.  Part of my decision process is related to
>> the next part of this reply.
>
> Here’s the updated webrev:
> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.02/
>
> Need a second reviewer, in addition to waiting for Stefan.
>

Looks good.

Thanks,
StefanK
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
> On Apr 19, 2017, at 6:01 AM, Stefan Karlsson <[hidden email]> wrote:
>
> On 2017-04-19 08:40, Kim Barrett wrote:
>> Here’s the updated webrev:
>> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.02/
>>
>> Need a second reviewer, in addition to waiting for Stefan.
>>
>
> Looks good.

Thanks.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Chris Plummer
In reply to this post by Kim Barrett
On 4/18/17 11:40 PM, Kim Barrett wrote:

>> On Apr 11, 2017, at 1:45 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On Apr 10, 2017, at 2:21 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>>> On Apr 10, 2017, at 12:48 PM, Stefan Karlsson <[hidden email]> wrote:
>>>> To be consistent with the other code it in that file (res & 1) == 0 should be !(res & 1).
>>> That would be contrary to hotspot style guide; I think that should take precedence.
>> Stefan and I discussed this privately, and I�ve agreed to go with the locally consistent style here,
>> even though it�s contrary to the hotspot style guide.  Part of my decision process is related to
>> the next part of this reply.
> Here�s the updated webrev:
> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.02/
>
> Need a second reviewer, in addition to waiting for Stefan.
>
Hi Kim,

Looks good.

Chris


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178352: BitMap::get_next_zero_offset may give wrong result on Mac

Kim Barrett
> On May 3, 2017, at 12:42 AM, Chris Plummer <[hidden email]> wrote:
>
> On 4/18/17 11:40 PM, Kim Barrett wrote:
>>> On Apr 11, 2017, at 1:45 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>>> On Apr 10, 2017, at 2:21 PM, Kim Barrett <[hidden email]> wrote:
>>>>
>>>>> On Apr 10, 2017, at 12:48 PM, Stefan Karlsson <[hidden email]> wrote:
>>>>> To be consistent with the other code it in that file (res & 1) == 0 should be !(res & 1).
>>>> That would be contrary to hotspot style guide; I think that should take precedence.
>>> Stefan and I discussed this privately, and I�ve agreed to go with the locally consistent style here,
>>> even though it�s contrary to the hotspot style guide.  Part of my decision process is related to
>>> the next part of this reply.
>> Here�s the updated webrev:
>> http://cr.openjdk.java.net/~kbarrett/8178352/hotspot.02/
>>
>> Need a second reviewer, in addition to waiting for Stefan.
>>
> Hi Kim,
>
> Looks good.
>
> Chris

Thanks!

Loading...