RFR: JDK-8077265 Modify assert to help debug JDK-8068448

[Eric Caspole]

Hi, here's a new webrev with the new assert in psOldGen as a function -
I capture only the covered_region in a local as it directly relates to 
the problem.

  http://cr.openjdk.java.net/~ecaspole/JDK-8077265/02/webrev/

Passes JPRT.
Thanks,
Eric


On 4/13/2015 4:39 PM, Stefan Karlsson wrote:
> On 2015-04-13 17:52, Eric Caspole wrote:
>> Thinking about it over the weekend, I really do want some new asserts 
>> in psOldGen because it should catch the problem in a 
>> different/interesting way.
>> I am happy to back out the  changes in psPromotionLAB.cpp and hpp, 
>> since that part already catches the problem.
>>
>> Stefan, do you want to this change to use the macro in psOldGen as I 
>> was doing before or refactor into a new function?
>
> I think I'd prefer a new function to get rid of the setup of the local 
> __variable_name__ variables. We would lose the line number of the call 
> site, if we replace the macro with a function, but the stack trace in 
> the hs_err file tell us where we came from, so I think that's OK.
>
> Thanks,
> StefanK
>
>
>> Thanks,
>> Eric
>>
>>
>> On 4/10/2015 9:19 AM, Stefan Karlsson wrote:
>>> On 2015-04-10 15:15, Eric Caspole wrote:
>>>> I don't want to make sweeping changes to try to debug what seems 
>>>> like a race.
>>>> If you dont like the macro in psOldGen I will remove it and just go 
>>>> on with the original one.
>>>
>>> Sounds good to me.
>>>
>>> StefanK
>>>
>>>> Eric
>>>>
>>>>
>>>> On 4/10/2015 4:43 AM, Stefan Karlsson wrote:
>>>>> On 2015-04-09 22:17, Eric Caspole wrote:
>>>>>> Hi everybody,
>>>>>> I updated this so the psOldGen part use a macro as Stefan suggested.
>>>>>> The assert in psPromotionLAB.hpp is allocating out of an already 
>>>>>> allocated PLAB, so I don't think that one will ever be hit but I 
>>>>>> want it there just in case.
>>>>>> And as Jesper suggested I made the message more helpful in the 
>>>>>> original place.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ecaspole/JDK-8077265/01/webrev/
>>>>>
>>>>> I had hoped you would use the new define for all assert, but 
>>>>> you've only used it for two out of four. If you're not going to 
>>>>> use the same macro for all usages, it doesn't seem worth having.
>>>>>
>>>>> http://cr.openjdk.java.net/~ecaspole/JDK-8077265/01/webrev/src/share/vm/gc_implementation/parallelScavenge/psOldGen.hpp.udiff.html 
>>>>>
>>>>>
>>>>> It seems to me that we could refactor allocate_noexpand and 
>>>>> cas_allocate_noexpand to use a common function to do the assert 
>>>>> and update the start array. That way you could get rid of the 
>>>>> macro you added.
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>>
>>>>>> Passes JPRT.
>>>>>> Thanks,
>>>>>> Eric
>>>>>>
>>>>>> On 4/9/2015 10:01 AM, Stefan Karlsson wrote:
>>>>>>> Hi Eric,
>>>>>>>
>>>>>>> On 2015-04-09 15:19, Eric Caspole wrote:
>>>>>>>> HI everybody,
>>>>>>>> Here is a webrev to add more asserts related to debugging 
>>>>>>>> JDK-8068448. Beyond capturing more info in the original assert, 
>>>>>>>> after looking at another core I added more asserts to make sure 
>>>>>>>> there is no other place where old gen allocations would overrun 
>>>>>>>> the start array.
>>>>>>>
>>>>>>> Why didn't these two new asserts get the same, more informative, 
>>>>>>> error message as the first assert you changed? Maybe you could 
>>>>>>> extract the check out to a helper macro that prints the relevant 
>>>>>>> information?
>>>>>>>
>>>>>>> Another point that Bengt mentioned yesterday, is that we don't 
>>>>>>> really need to print the old_gen part of the assert. It's 
>>>>>>> already printed in the hs_err file.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> StefanK
>>>>>>>
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ecaspole/JDK-8077265/00/webrev/
>>>>>>>>
>>>>>>>> Passes JPRT.
>>>>>>>> Thanks,
>>>>>>>> Eric
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150414/80f00791/attachment.htm>