Looks fine to me.  Thanks!

Xuelei

On 9/22/2016 4:05 PM, Jamil Nimeh wrote:
That's a very good suggestion.  I've made that change and updated the
webrev:

http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.02/

Thanks again,
--Jamil

On 09/21/2016 05:34 PM, Xuelei Fan wrote:
> latch = (latch + 1) & 0x7FFFFFFF;  // Mask the sign bit
I'm fine with it.

BTW, if you like, I may suggest to use a little bit small number for
the mask, for example 0x1FFFFFFF, so that the counter variable does
not overflow either.  It works if counter overflow, but better not if
we want the counter meaningful.

Xuelei


On 9/22/2016 7:29 AM, Jamil Nimeh wrote:
So here are a couple ideas related to your suggestion.

We can leave a simple increment on latch and let it overflow, then in
the array access do this:

v ^= rndTab[(latch & 0x7FFFFFFF) % 255];

That clears the high order bit and the mod will keep it in the range of
0-254.  the lvalue for the mod operation will never go negative.  The
downside of that is counter will still get assigned a large negative
value (which as I said earlier could cause another iteration of the main
loop...not the end of the world since it can only happen 6 times max).

The other approach would be to increment latch and mask off the high
order bit:

latch = (latch + 1) & 0x7FFFFFFF;  // Mask the sign bit

Then the v^=[latch % 255] is safe as-is and counter doesn't get some
massively negative value (though it can still overflow across multiple
iterations of the outer loop).

I like the second approach a little better, personally. Up-front use of
the bitwise-and is a little more clear that we're forcing the value to
be non-negative before we use it as an array index input.  Let me know
what you think and I'll update the webrev accordingly.

Thanks,
--Jamil

On 09/21/2016 09:05 AM, Wang Weijun wrote:
I am OK with your fix, but I found "(latch + 1) % Integer.MAX_VALUE" a
little difficult to understand. Does rndTab[latch & 0xff] also work?

Thanks
Max


On Sep 21, 2016, at 11:57 PM, Jamil Nimeh <jamil.j.ni...@oracle.com>
wrote:

Hi Max and Xuelei,

Yesterday I also reached out to the SQE engineer who submitted the
bug, asking if this is an issue he's seen going forward from the
original instance in 8u20.  He said that he hasn't seen this issue
come up since the original bug submission.  Since the simple overflow
condition is easily solved with my fix, and the code has been
otherwise stable I'd like to suggest that we keep the fix as-is.  The
loop timing as it stands now is not the source of the bug, other than
that latch can overflow and that is solved in one line.  If we want
to revisit this and improve the overall performance (though I haven't
seen evidence that there is a perf issue with this at all) then maybe
an RFE is in order.  What do you think?

--Jamil


Reply via email to