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
On 09/20/2016 10:32 PM, Jamil Nimeh wrote:
Hi Max and Xuelei, thanks for the feedback.
On 09/20/2016 07:52 PM, Wang Weijun wrote:
On Sep 21, 2016, at 9:58 AM, Xuelei Fan <xuelei....@oracle.com> wrote:
359 while (System.nanoTime() - startTime < 250000000) {
360 synchronized(this){};
- 361 latch++;
+ 361 latch = (latch + 1) % Integer.MAX_VALUE;
362 }
This block may be not CPU friendly as it may loop a large amount of
times in a very short period (250milli).
You asked about the empty synchronized block also: From what I've been
reading on this topic it looks like the use of the empty synchronized
block can be used to force cache coherency between multiple threads.
In terms of it being CPU intensive, has seed generation ever pegged a
processor in the past?
There were cases in the past where it would hang, but that was fixed
back when Max changed things in the inner loop to use
System.nanoTime() (see JDK-8157318) but at that point (only 3 months
ago) we didn't feel the need to restructure the loop. I don't know
that we do at this point either. But we certainly can fix the
overflow of the latch easily enough.
To get a <255 index I think we only need to loop for <66536 times.
How about we stop at every millisecond and see if it's enough?
Something like this:
long next = startTime + 1000000;
while (next < startTime + 250000000) {
while (System.nanoTime() < next) {
synchronized(this){};
latch++;
}
if (latch > 65535 || latch < 0) break;
next += 1000000;
}
What's the usage of line 360? Just for some computation?
367 counter += latch;
The counter variable may be overflow too.
I find this strange. Were computers so slow in 1996 that within 250ms
latch cannot exceed 64000?
1996? You're talking about pentium and pentium 2 machines so at best
you're talking 450MHz. I don't know if the latch wouldn't pop under
those conditions.
As for the counter, a potential overflow I don't think is that bad
given the way the loop control is written. At worst it just means
another spin around the outer loop and another byte dropped in the
pool. And that loop can only iterate 6 times at the most so it's not
like things can run away.
--Max
Xuelei
On 9/21/2016 8:57 AM, Jamil Nimeh wrote:
Hello all,
This fixes a bug found in stress testing where on faster CPUs the
latch
can overflow resulting in a negative array index. The fix avoids the
overflow by resetting the latch to 0 when it reaches
Integer.MAX_VALUE -
1 and will continue increasing from there.
Bug: https://bugs.openjdk.java.net/browse/JDK-8049516
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.01/
Thanks,
--Jamil
