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

[Jon Masamitsu]

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