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

[Jon Masamitsu]

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

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