Request for review (xs) 8006040: NPG: on_stack processing wastes space in ConstantPool

[Coleen Phillimore]

On 01/22/2013 03:38 PM, Stefan Karlsson (Oracle) wrote:
> On Jan 22, 2013, at 8:21 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>
>> David,
>>
>> Thank you for noticing my code review!!   I have an update at
>> http://cr.openjdk.java.net/~coleenp/8006040/

I meant http://cr.openjdk.java.net/~coleenp/8006040_2/  was the update.
>
> You removed the atomic operations with the comment that the invokedynamic bits doesn't require atomic operations. What about the other bits, are they ever written to concurrently? If not, then it looks good.

All three of the existing bits were invokedynamic bits, and I checked 
with Chris and John Rose.   They confirmed that they didn't need to be 
atomic.  Two of them didn't have any effect other than printing.

Thanks!
Coleen
>
> StefanK
>
>> that addresses  your comments.
>> I need another reviewer!
>>
>> On 01/20/2013 09:29 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> constantPool.cpp:
>>>
>>>    // all fields are initialized; needed for GC
>>> -   set_on_stack(false);
>> I don't think this comment means anything anymore.   I think it had something to do with the constantPoolOop object being partially initialized when hitting a GC point before Permgen was eliminated. I'll remove the comment also.
>>
>>> Isn't the comment tied to the now deleted line?
>>>
>>> !   if (has_pseudo_string() || has_invokedynamic() || has_preresolution()) {
>>>       if (has_pseudo_string()) st->print(" has_pseudo_string");
>>>       if (has_invokedynamic()) st->print(" has_invokedynamic");
>>>       if (has_preresolution()) st->print(" has_preresolution");
>>>       st->cr();
>>>     }
>>>
>>> The double-checking of the conditions here is not nice.
>> I had this change for a previous version so that the flags could be all on one line (and _flags was removed in my previous version).  I restored the code and added printing the on_stack flag too.
>>
>> Coleen
>>
>>> Otherwise flag changes look good, as do moving MetadataMarkOnStack segements.
>>>
>>> Thanks,
>>> David
>>>
>>> On 19/01/2013 2:41 AM, Coleen Phillimore wrote:
>>>> This is relatively easy, anyone? Also, it doesn't affect the SA. I checked.
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 1/16/2013 4:23 PM, Coleen Phillimore wrote:
>>>>> Summary: Added on_stack bit to _flags. Also MetadataMarkOnStack is
>>>>> used for more than JVMTI so had to be moved.
>>>>>
>>>>> Confirmed with John and Chris that setting invokedynamic bits doesn't
>>>>> require atomic operations so I can add on_stack to the flags.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8006040/
>>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8006040
>>>>>
>>>>> Tested NSK quick.testlist and runThese.
>>>>>
>>>>> Thanks,
>>>>> Coleen