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

[Bengt Rutisson]

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().

>
> 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.

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
>>>
>>
>