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

[Bengt Rutisson]

Thanks, Mikael!

Bengt

On 2015-04-10 15:08, Mikael Gerdin wrote:
> Bengt,
>
> On 2015-04-10 14:34, 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/
>
> Looks good to me.
> /Mikael
>
>>
>> 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