Request for review (S): 7110718 -XX:MarkSweepAlwaysCompactCount=0 crashes the JVM
John Cuthbertson
john.cuthbertson at oracle.com
Fri Nov 18 17:11:09 UTC 2011
Hi Bengt,
The .03 version looks good to me. One minor nit - can you place
parentheses around the MarkSweepAlwaysCompactCount < 1 expressions? O
know they're not required but, to my eye at least, thy help separating
the expressions used in the or.
Thanks,
JohnC
On 11/17/11 23:53, Bengt Rutisson wrote:
>
> 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/2650b553/attachment.htm>
More information about the hotspot-gc-dev
mailing list