Request for reviews (XS): 7048030: is_scavengable changes causing compiler to embed more constants

[John Rose]

Yes; thanks.  You can count me as a reviewer.  -- John

On May 24, 2011, at 6:36 PM, Vladimir Kozlov wrote:

> Tom Rodriguez wrote:
>> On May 24, 2011, at 6:00 PM, John Rose wrote:
>>> If this affects the processing of 292 data, there may be significant performance regressions.  The key uses of embedded non-perms are invokedynamic CallSites, which requires TypeOopPtr::make_from_constant to succeed for user-provided CallSite objects.
> 
> make_from_constant() is called with require_constant == true for invokedynamic CallSites and it calls can_be_constant() which will return true for ScavengeRootsInCode >= 1. So it should continue to work for 292.
> 
> Vladimir
> 
>>> 
>>> But, I think this will continue to work if ScavengeRootsInCode is not zero.  Do you agree?
>> It should restore the logic to how it used to work prior to 7041789.
>> http://hg.openjdk.java.net/jdk7/hotspot-gc/hotspot/raw-diff/2aa9ddbb9e60/src/share/vm/gc_interface/collectedHeap.hpp
>> @@ -284,11 +291,7 @@ class CollectedHeap : public CHeapObj {
>>    // An object is scavengable if its location may move during a scavenge.
>>   // (A scavenge is a GC which is not a full GC.)
>> -  // Currently, this just means it is not perm (and not null).
>> -  // This could change if we rethink what's in perm-gen.
>> -  bool is_scavengable(const void *p) const {
>> -    return !is_in_permanent_or_null(p);
>> -  }
>> +  virtual bool is_scavengable(const void *p) = 0;
>> 292 ignores should_be_constant anyway.
>> tom
>>> -- John
>>> 
>>> P.S.  FTR, some of history behind is_scavengable and is_perm is in this discussion, relating to 6863023:
>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2009-September/001261.html
>>> 
>>> On May 24, 2011, at 5:19 PM, Vladimir Kozlov wrote:
>>> 
>>>> http://cr.openjdk.java.net/~kvn/7048030/webrev
>>>> 
>>>> Fixed 7048030: is_scavengable changes causing compiler to embed more constants
>>>> 
>>>> This is the cause of 7047300 problem to appear.  The fix for 7041789 corrected the meaning of is_scavengable to really mean what it says.  Unfortunately there were some places that were really using it as a proxy for !is_perm.  In particular in ciObject::can_be_constant and should_be_constant we will now embed constants that we didn't used as long as they are in tenured.  The code should be changed from !is_scavengable to is_perm.
>>>> 
>>>> Verified with failing test from 7047300.