Request for review (S): 7110718 -XX:MarkSweepAlwaysCompactCount=0 crashes the JVM
Bengt Rutisson
bengt.rutisson at oracle.com
Mon Nov 21 06:58:10 UTC 2011
Hi John,
On 2011-11-18 18:11, John Cuthbertson wrote:
> 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.
Good point. I'll add the parentheses.
All set to push this now. Thanks Ramki, Jon, John and Tony for the reviews!
Bengt
>
> 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/20111121/713f0363/attachment.htm>
More information about the hotspot-gc-dev
mailing list