Request for review (S): 7110718 -XX:MarkSweepAlwaysCompactCount=0 crashes the JVM
Bengt Rutisson
bengt.rutisson at oracle.com
Fri Nov 18 07:53:01 UTC 2011
Ramki,
Thanks for the extra testing!
Jon, John and Tony,
Thanks for the reviews! Are you OK with pushing this (handle values < 1
as never force full compaction):
http://cr.openjdk.java.net/~brutisso/7110718/webrev.03/
Instead of this (forbid values < 1):
http://cr.openjdk.java.net/~brutisso/7110718/webrev.02/
I think your review comments were more related to webrev.02, so I would
like to double check with you before I push my changes in webrev.03.
Thanks,
Bengt
On 2011-11-18 08:11, Srinivas Ramakrishna wrote:
> 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 <mailto:bengt.rutisson at oracle.com>> wrote:
>
>
> Hi Ramki,
>
> Is this what you were considering?
> http://cr.openjdk.java.net/~brutisso/7110718/webrev.03/
> <http://cr.openjdk.java.net/%7Ebrutisso/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/20111118/b37eb1d1/attachment.htm>
More information about the hotspot-gc-dev
mailing list