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

[Jon Masamitsu]

Bengt,

I think that the code using VerifyUpdateClosure is dead.  I'll file a CR to
remove it.

Jon

On 11/17/11 06:13, Bengt Rutisson 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 <mailto: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 <mailto: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/
>>>         <http://cr.openjdk.java.net/%7Ebrutisso/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/1eff2958/attachment.htm>