Request for code review (M) - 8081629: CMS split_block() does not correctly fix up block-offset-table for large blocks

[Jon Masamitsu]

Ramki,

On 09/10/2015 12:05 AM, Srinivas Ramakrishna wrote:
> Hi Jon --
>
> Sorry again for the delay in looking at the second version of your 
> fix. I had a look at that second fix. I do like the simplificatio
> of the loop that finds the first power block to fix, but I think the 
> rest of the code is, at least to me, clearer in the first fix that you 
> have.
>
> Thus, I am more inclined to support going with the first fix, rather 
> than the second, unless you feel very strongly otherwise, in which
> case I will take a second look at your second fix and perhaps suggest 
> further changes. Let me know if you want to go that route,
> rather than going with the minimal first fix.

I'll use the minimal fix.

>
> Also, it might be nice to backport this fix to JDK 8 if possible, 
> since applications that use CMS and allocate large objects may run into
> this performance problem. (e.g. We have recently had a case where this 
> seems to have contributed to a slight slow down in
> ParNew -- nothing as dramatic as that demonstrated by the test case in 
> the companion ticket, but still sufficiently large and
> long-lasting to be of concern.)

The process is fuzzy in my mind but I don't think
there is going to be another 8 update release (which
is the easy path for a bug fix) so a backport might
require an escalation to get the sustaining organization
involved.

Thanks for the review.

Jon



> thanks!
> -- ramki
>
>
> On Mon, Aug 31, 2015 at 9:25 AM, Srinivas Ramakrishna 
> <ysr1729 at gmail.com <mailto:ysr1729 at gmail.com>> wrote:
>
>     Hi Jon --
>
>     Sorry for the looong delay in getting to this and then of turning
>     around the webrev once I started looking at the code.
>     It's been a while since I was in this code, and the code is quite
>     complicated, so it took a while to swap back
>     the context and understand (the correctness of) the change.
>
>
>
>     On Mon, Jun 22, 2015 at 12:57 PM, Jon Masamitsu
>     <jon.masamitsu at oracle.com <mailto:jon.masamitsu at oracle.com>> wrote:
>
>         This bug is causing large young GC times for ParNew when
>         there are large objects in the heap and the logarithmic strides
>         are being used (badly).
>
>         https://bugs.openjdk.java.net/browse/JDK-8081629
>
>         The change in webrev_min corrects the problem
>         with the minimum code change (least risk).
>
>         http://cr.openjdk.java.net/~jmasa/8081629/webrev_min.02/
>         <http://cr.openjdk.java.net/%7Ejmasa/8081629/webrev_min.02/>
>
>
>
>     The above changes look good to me. Thanks also for confirming that
>     these
>     changes fix the assertion failure seen with
>     +VerifyBlockOffsetArray. It might be
>     worthwhile writing (in the future) a slightly more lightweight
>     VerifyBlockOffsetArray
>     so that it becomes practical to use.
>
>     I haven't had the opportunity to look at and understand the second
>     version of the
>     webrev below yet. But the one above looks good to me. I'll try and
>     get to the
>     version below in the next day or so. Sorry for the ongoing delay.
>
>     thanks!
>     -- ramki (openjdk: ysr)
>
>
>
>         A rewriting of split_block() in included in a second webrev
>
>         http://cr.openjdk.java.net/~jmasa/8081629/webrev.02/
>         <http://cr.openjdk.java.net/%7Ejmasa/8081629/webrev.02/>
>
>         I've willing to go with the minimum change but could also
>         be encouraged to push the rewrite of split_block().
>
>         Vote for the one you like with your code review.
>
>         The diff between webrev_min.02 and webrev.02 is
>         here but I don't find it that useful.
>
>         http://cr.openjdk.java.net/~jmasa/8081629/webrev_delta.02/
>         <http://cr.openjdk.java.net/%7Ejmasa/8081629/webrev_delta.02/>
>
>         Thanks.
>
>         Jon
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150910/f2bc01eb/attachment.htm>