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

Bengt Rutisson bengt.rutisson at oracle.com
Fri Apr 10 12:34:26 UTC 2015


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


More information about the hotspot-gc-dev mailing list