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

[Srinivas Ramakrishna]

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.

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.)

thanks!
-- ramki


On Mon, Aug 31, 2015 at 9:25 AM, Srinivas Ramakrishna <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>
> 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/
>
>
>
> 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/
>>
>> 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/
>>
>> Thanks.
>>
>> Jon
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150910/c5173341/attachment.htm>