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

[Jon Masamitsu]

On 4/12/2015 6:29 AM, Bengt Rutisson wrote:
>
> 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.

OK.

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

Having an is_in_for_asserts() would allow you to focus on the performance
critical uses.  I would use is_in_reserved() for any performance 
critical uses.

If is_in() is only defined under #ifdef ASSERT,  are there less than a 
dozen
place that need to be fixed?

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

The specification for is_in() makes it clear that it should not be used in
performance sensitive code so I'm also not sure what a good rename
would be or if a rename would help that much.

   // Returns "TRUE" iff "p" points into the committed areas in the 
generation.
   // For some kinds of generations, this may be an expensive operation.
   // To avoid performance problems stemming from its inadvertent use in
   // product jvm's, we restrict its use to assertion checking or
   // verification only.

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

True, G1 and Parallel are the collectors that get performance
enhancements these days, but I have spent some time investigating
regression in CMS young pause times.  Removing the guarantee
doesn't make such investigations easier.

Jon

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