<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Stefan,<br>
<br>
> Full: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.01/">http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.01/</a>
<br>
<br>
The fix looks good to me. Some minor comments:<br>
<br>
<pre>
<span class="changed"><span class="changed">310 storage.setArrayAt((to * K + k), new Object[N]);
</span>--></span><span class="changed"></span><span class="changed"><span class="changed">
310 storage.setArrayAt(to * K + k, new Object[N]);</span></span>
<span class="changed"><span class="changed"></span></span></pre>
<br>
<pre><span class="new">355 public Object[][] storage;
-->
</span><span class="new">355 public final Object[][] storage;
</span><span class="new">363 Object[] getForIndex(int y) {</span>
<span class="new">364 return storage[y % usedCount];</span>
<span class="new">365 }
- no need this method.
</span>
<span class="new"><span class="new">368 return usedCount == storage.length;</span>
-->
</span><span class="new"><span class="new">368 return usedCount >= storage.length;</span>
</span>
I don't need a separate review for that tiny changes.
Thanks,
Dima
</pre>
<br>
<div class="moz-cite-prefix">On 27.05.2016 15:48, Stefan Johansson
wrote:<br>
</div>
<blockquote cite="mid:57484215.6060606@oracle.com" type="cite">
<br>
<br>
On 2016-05-27 13:33, Mikael Gerdin wrote:
<br>
<blockquote type="cite">Hi Stefan,
<br>
<br>
On 2016-05-26 17:20, Stefan Johansson wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
Please review this testfix for:
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8157153">https://bugs.openjdk.java.net/browse/JDK-8157153</a>
<br>
<br>
Webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.00/">http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.00/</a>
<br>
</blockquote>
<br>
Would it be possible to encapsulate the storage[] so that you
don't have to put the "% storageUsedCount" all over the place?
<br>
This seems a bit fragile to me.
<br>
</blockquote>
Thanks for taking a look Mikael. I agree, that my initial fix
might cause problems and I like how your idea turned out:
<br>
Full: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.01/">http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.01/</a>
<br>
Inc: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.00-01/">http://cr.openjdk.java.net/~sjohanss/8157153/hotspot.00-01/</a>
<br>
<br>
Thanks,
<br>
Stefan
<br>
<br>
<blockquote type="cite">
<br>
/Mikael
<br>
<br>
<blockquote type="cite">
<br>
Summary:
<br>
The test sometimes fail due to running out of memory and the
reason is
<br>
that the test expects to be able to fit 2 objects of the size
<br>
HeapRegionSize/2 into one heap region. This assumption is ok
in theory,
<br>
but if any object at all is allocated between the two
allocations the
<br>
second one will end up in a different region. The test
calculates how
<br>
many regions it should fill to get 90% of the heap used and
then
<br>
allocated twice this number of objects. In many cases this
will work
<br>
since nothing else is going on, but in some cases some other
subsystem
<br>
might start allocating objects, and this destroys the
assumptions
<br>
leading to hitting an OOME.
<br>
<br>
The fix simply adds a second check to the allocation loop to
stop as
<br>
soon as the free memory drops below 10% of the heap, this also
requires
<br>
some extra limiting of the indexes used.
<br>
<br>
Testing:
<br>
* Local testing as well as runs through RBT.
<br>
<br>
Thanks,
<br>
Stefan
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>