RFR (S): 8077403: Remove guarantee from GenCollectedHeap::is_in()

Bengt Rutisson bengt.rutisson at oracle.com
Tue Apr 14 09:45:23 UTC 2015


Hi Jon,

Thanks for looking at this and taking the time to discuss it!

Bengt

On 2015-04-14 04:10, Jon Masamitsu wrote:
> Bengt,
>
> On 4/13/2015 1:49 PM, Bengt Rutisson wrote:
>> On 13/04/15 18:53, Jon Masamitsu wrote:
>>>
>>>
>>> On 4/12/2015 6:29 AM, Bengt Rutisson wrote:
>>>>
>>>> Hi Jon,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> On 10/04/15 20:24, Jon Masamitsu wrote:
>>>>> Bengt,
>>>>>
>>>>> I added a comment to the CR with some of the history (as I recall 
>>>>> it),
>>>>> about the desired restriction for the is_in() method.
>>>>
>>>> Thanks for adding this background.
>>>>
>>>> If it turns out that young_gen->is_in() needs to be used in a 
>>>> performance critical place then maybe we can optimize it? If we 
>>>> bypass the generation abstraction the young gen check could be 
>>>> turned in to two simple range checks instead of setting up space 
>>>> iterators and doing virtual calls as it is now. I don't think that 
>>>> work is useful to do right now. We have avoided the use of this 
>>>> method in performance critical code and Jesper is cleaning up the 
>>>> generation abstraction. We should wait until he is finished with 
>>>> the cleanups.
>>>
>>> OK.
>>>
>>>>> Would it
>>>>> work to create an is_in_for_asserts() that would only be defined
>>>>> under ASSERT to check that the uses were as expected?  I know it
>>>>> will not catch all the uses, but maybe those that are not covered
>>>>> do not need the preciseness of is_in() and could use is_in_reserved()
>>>>> instead.
>>>> The reason the guarantee was there in the first place was that the 
>>>> is_in() method is not just called from asserts. I can add an 
>>>> is_in_for_asserts() method, but I still have to do something for 
>>>> the places (verification and dump code) that are in product code. 
>>>> So, I am not sure how adding a is_in_for_asserts() will help. Do 
>>>> you have a suggestion?
>>>
>>> Having an is_in_for_asserts() would allow you to focus on the 
>>> performance
>>> critical uses.  I would use is_in_reserved() for any performance 
>>> critical uses.
>>
>> But is_in() is used in several non-performance critical places in 
>> product code. Such as the dump code. I don't think we want to replace 
>> those with is_in_reserved().
>
> I see two places where you're right that is_in_reserved() would not serve
> as well.
>
> share/vm/runtime/os.cpp
>
>    936    if (Universe::heap()->is_in(addr)) {
>    937      HeapWord* p = Universe::heap()->block_start(addr);
>
> and
>
> share/vm/utilities/debug.cpp
>
>    510  extern "C" void pp(void* p) {
>    511    Command c("pp");
>    512    FlagSetting fl(PrintVMMessages, true);
>    513    FlagSetting f2(DisplayVMOutput, true);
>    514    if (Universe::heap()->is_in(p)) {
>
> so I will yield that point.
>
>>
>>>
>>> If is_in() is only defined under #ifdef ASSERT,  are there less than 
>>> a dozen
>>> place that need to be fixed?
>>
>> There are not many places. I just don't know how to fix them. I think 
>> we don't want to loose precision and go to is_in_reserved(). So, I 
>> think we want something like is_in() also for product code. That's 
>> why I don't know what good a is_in_for_asserts() would be.
>>
>>>
>>>>
>>>> I was thinking about renaming is_in() to is_in_slow() to signal to 
>>>> users that it should nob be used in performance critical code. But 
>>>> that sounds like a check for something that is in a slow area. I'm 
>>>> open for naming suggestions.
>>>
>>> The specification for is_in() makes it clear that it should not be 
>>> used in
>>> performance sensitive code so I'm also not sure what a good rename
>>> would be or if a rename would help that much.
>>>
>>>   // Returns "TRUE" iff "p" points into the committed areas in the 
>>> generation.
>>>   // For some kinds of generations, this may be an expensive operation.
>>>   // To avoid performance problems stemming from its inadvertent use in
>>>   // product jvm's, we restrict its use to assertion checking or
>>>   // verification only.
>>
>> Agreed. So, I'll leave the name as it is.
>>>
>>>>
>>>> Also, note that the guarantee I remove was only present in 
>>>> GenCollectedHeap. G1 and Parallel does not do this check. So, I 
>>>> don't see how this guarantee helped us much since G1 and Parallel 
>>>> is where we mostly work on performance.
>>>
>>> True, G1 and Parallel are the collectors that get performance
>>> enhancements these days, but I have spent some time investigating
>>> regression in CMS young pause times.  Removing the guarantee
>>> doesn't make such investigations easier.
>>
>> Do you think the guarantee is worth it even though it leaks all kinds 
>> of strange code in to the is_in() implementation, such as 
>> PrintAssembly for example? To me the long list of exceptions in the 
>> guarantee is an indication that it is not the right approach to ease 
>> performance investigations.
>
> I agree that what's there if far from perfect.  I won't object to the 
> change.
>
> Jon
>
>>
>> Bengt
>>
>>>
>>>
>>> Jon
>>>
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>>>
>>>>> Jon
>>>>>
>>>>> On 4/10/2015 5:34 AM, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi everyone,
>>>>>>
>>>>>> Can I have a couple of reviews for this small change?
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8077403
>>>>>> http://cr.openjdk.java.net/~brutisso/8077403/webrev.00/
>>>>>>
>>>>>> The method CollectedHeap::is_in() has the following comment:
>>>>>>
>>>>>>   // Returns "TRUE" iff "p" points into the committed areas of 
>>>>>> the heap.
>>>>>>   // Since this method can be expensive in general, we restrict its
>>>>>>   // use to assertion checking only.
>>>>>>
>>>>>> This is incorrect since we don't just use it for asserts. We also 
>>>>>> use it in os::print_location() which is in turn is used by 
>>>>>> several different code paths, such as verification, crash dumping 
>>>>>> and logging.
>>>>>>
>>>>>> In GenCollectedHeap::is_in() there is an attempt to verify that 
>>>>>> we only call this method from places that are not performance 
>>>>>> critical. So, its implementation looks like this:
>>>>>>
>>>>>> bool GenCollectedHeap::is_in(const void* p) const {
>>>>>>   #ifndef ASSERT
>>>>>>   guarantee(VerifyBeforeGC ||
>>>>>>             VerifyDuringGC ||
>>>>>>             VerifyBeforeExit ||
>>>>>>             VerifyDuringStartup ||
>>>>>>             PrintAssembly ||
>>>>>>             tty->count() != 0 || // already printing
>>>>>>             VerifyAfterGC ||
>>>>>>     VMError::fatal_error_in_progress(), "too expensive");
>>>>>>
>>>>>>   #endif
>>>>>>   return _young_gen->is_in(p) || _old_gen->is_in(p);
>>>>>> }
>>>>>>
>>>>>> It is quite odd that we do this check here and even more strange 
>>>>>> that it is only done in GenCollectedHeap and not in the other 
>>>>>> implementations (G1CollectedHeap and ParallelScavengeHeap).
>>>>>>
>>>>>> I doubt that this check is useful. The method is actually not 
>>>>>> that slow. It basically just iterates over the three spaces in 
>>>>>> young gen to do range checks and then does a single range check 
>>>>>> on the old gen. So, 4 range checks all together.
>>>>>>
>>>>>> Rather than having GenCollectedHeap::is_in() know about all 
>>>>>> callers of it I think we should remove the guarantee.
>>>>>>
>>>>>> The method CollectedHeap::is_in() is also called by three 
>>>>>> versions of is_in_place() but these three methods are unused, so 
>>>>>> I remove them as part of this patch.
>>>>>>
>>>>>> The method CollectedHeap::is_in_or_null() is only used in 
>>>>>> asserts, so I made it DEBUG_ONLY.
>>>>>>
>>>>>> I also make two style change to ParallelScavengeHeap. I made the 
>>>>>> implementation of ParallelScavengeHeap::is_in() look more like 
>>>>>> the implementation of GenCollectedHeap::is_in(). That made 
>>>>>> ParallelScavengeHeap::is_in_reserved() stick out, so I made that 
>>>>>> look cosistent with the new version of 
>>>>>> ParallelScavengeHeap::is_in().
>>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>>
>>>>
>>>
>>
>



More information about the hotspot-gc-dev mailing list