Request for review (S): 7110718 -XX:MarkSweepAlwaysCompactCount=0 crashes the JVM

[Srinivas Ramakrishna]

Hi Bengt -- this looks good to me. It also passed my testing...
I also confirmed what Jon indicated -- that VerifyUpdateClosure is dead
code, as far as my cscope
navigation showed and can be deleted (in a separate CR as Jon said).

Rebiewed; thanks!
-- ramki

On Thu, Nov 17, 2011 at 6:13 AM, Bengt Rutisson
<bengt.rutisson at oracle.com>wrote:

> **
>
> Hi Ramki,
>
> Is this what you were considering?
> http://cr.openjdk.java.net/~brutisso/7110718/webrev.03/
>
> I think it should interpret MarkSweepAlwaysCompactCount <= 0 as "never
> force full compaction". It would be great if you could do some testing.
> There is one assert in PSParallelCompact::VerifyUpdateClosure::do_addr()
> that worries me a bit:
>
>   assert(HeapMaximumCompactionInterval > 1 || MarkSweepAlwaysCompactCount
> > 1 ||
>          forwarding_ptr == new_pointer, "new location is incorrect");
>
> I think this should be safe since MarkSweepAlwaysCompactCount == 1 was an
> acceptable value even before my change. But I have to admit that I don't
> really understand what the assert is trying to verify.
>
> Bengt
>
>
>
> On 2011-11-16 19:01, Srinivas Ramakrishna wrote:
>
> Hi Bengt, Not sure how much customers use this option. Its useful for
> "serviceability in production" kind of scenarios
> to have the code be more robust. I think it would be useful. I appreciate
> the need for more testing
> of course, and I am happy to do that testing for you -- just let me know
> and I'll grab yr patch and test
> here.
>
> thanks!
> -- ramki
>
> On Tue, Nov 15, 2011 at 6:50 AM, Bengt Rutisson <bengt.rutisson at oracle.com
> > wrote:
>
>>
>> Ramki,
>>
>>
>> On 2011-11-14 20:32, Srinivas Ramakrishna wrote:
>>
>>
>> Thanks, Bengt, for the super-quick turnaround!! A comment below on the
>> choice of <= 0 for the option value....
>>
>>
>>  Thanks for the review! See comments below.
>>
>>
>>  On Mon, Nov 14, 2011 at 1:25 AM, Bengt Rutisson <
>> bengt.rutisson at oracle.com> wrote:
>>
>>>
>>> Hi all,
>>>
>>> Can I have a couple of reviews for this small change?
>>> http://cr.openjdk.java.net/~brutisso/7110718/webrev.01/
>>>
>>> It is a fix for the issue that Ramki reported recently.
>>> MarkSweepAlwaysCompactCount is used for division and Hotspot crashes if it
>>> is set to 0.
>>>
>>> I choose to log an error and exit the VM if someone tries to start with
>>> -XX:MarkSweepAlwaysCompactCount=0. An alternative is to just log a warning
>>> and set it to 1.
>>>
>>> I prefer the error way since it is not really clear what one wants to
>>> achieve with MarkSweepAlwaysCompactCount=0. Always do full compactions or
>>> never do full compactions? So I am not convinced that 1 is an appropriate
>>> value.
>>>
>>> Also, since the VM, up until now, has crashed if someone tried
>>> -XX:MarkSweepAlwaysCompactCount=0 I think we can be sure that there are no
>>> customers that are running with that setting. It should be safe to forbid
>>> it.
>>>
>>
>> I agree with that statement. However, given that the value 0 was
>> producing crashes, proving that no production code would have been using
>> that setting,
>> and based on yr comment above that the 0 value could as well have been
>> used to denote "never force full compaction", it seems as though an
>> alternative to exiting with
>> an error, is now to define all values <=0 to mean "never _force_ full
>> compaction"
>>
>> Especially since tolerating allowed input values and mapping them to
>> specific non-exiting behaviours allows us to modify production JVM's on the
>> fly
>> without causing loss of availability. (Consider a future in which this
>> option becomes a "manageable"; you would then be faced with the same
>> question, and it seems as though making this choice now would help
>> maintain consistency and robustness going forward -- we could of course
>> always throw a "illegal value exception" or such at that point, but
>> allowing the specification of "never _force_ full compaction" (unless the
>> JVM
>> otherwise chooses to) would appear to be a choice to allow users; mapping
>> negative and 0 values to that setting would avoid having to
>> throw an error.) However, I understand that this is somewhat subjective,
>> so I am willing to go with whatever the majority consensus here
>> mght be. It just seemed more pleasant to:
>> (1) allow the specification of reasonable behaviour (i.e. never _force_
>> ...)
>> (2) map the full domain of the option to a reasonable behaviour (i.e.
>> allow <= 0 to map to never _force_ ..)
>>
>> Comments?
>>
>>
>>  I see your point, and I think this should be fairly straight forward to
>> fix. However it will require some more testing etc. I can do that, but I
>> don't think I know enough to say whether or not the extra work is worth it.
>> How important is this option? Is it something that customers use a lot?
>>
>> Thanks,
>>  Bengt
>>
>>
>>
>> -- ramki
>>
>>
>>
>>>
>>> CR:
>>>
>>> 7110718 -XX:MarkSweepAlwaysCompactCount=0 crashes the JVM
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7110718
>>>
>>> Thanks,
>>>  Bengt
>>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20111117/ae83d580/attachment.htm>