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

[Mikael Gerdin]

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