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

[Bengt Rutisson]

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

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.

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.

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
>