Request for review (URGENT!!) 7033141: assert(has_cp_cache(i)) failed: oob

[Coleen Phillimore]

On 5/19/2011 11:17 AM, Karen Kinnear wrote:
> Coleen,
>
> Code looks good. Thank you for the latest update and testing.
Thanks Karen!
>
> Question - why did you remove the comments in instanceKlass.cpp lines 
> 368-375
> for rewrite_class? Are those three cases no longer true? I did think 
> there were perhaps
> more cases if you include redefineclasses and the new 
> rewrite/un-rewrite/rewrite on retry :-)
I removed the comment because I didn't understand it at all, so couldn't 
figure out how to correct it after spending a week with that code!  I 
think it was referring to old code for class data sharing might have 
gone back to call this rewrite_code function but in fact CDS just calls 
link_methods directly when loading shared classes out of the shared 
archive.  This is why I tested with CDS, just to make sure even though 
it didn't actually call this function.
>
> rewriter.cpp: line 412 - I think the comment would be "if there are 
> exceptions rewriting bytecodes
> or allocating the cpcache, not "or relocating bytecodes"
Thank you - I corrected that comment.

Coleen
>
> thanks,
> Karen
>
> On May 18, 2011, at 4:14 PM, Coleen Phillimore wrote:
>
>>
>> The last code change I sent out was not complete, and would not work 
>> if there was an exception in the relocator.  The code was structured as:
>>
>>   if klass->is_rewritten() flag {
>>       verify - exit if exception
>>       rewrite bytecodes to point to (to be created) cpCache rather 
>> than constant pool
>>       create constant pool cache  - (added restore_bytecodes and) 
>> exit if exception
>>       run relocator for jsr/ret    - exit if exception
>>       link method entry points  - exit if exception
>>       set rewritten flag
>>  }
>>
>> Any exceptions will cause the whole block to be retried if the 
>> exception is handled and class loading is attempted again for this 
>> class.  This latest code effectively changes this ordering to:
>>
>>   if klass->is_rewritten() flag {
>>     verify - exit if exception
>>     rewrite bytecodes to point to cpCache rather than constant pool 
>> entries
>>      create constant pool  {
>>          if exception restore bytecodes to point to constant pool
>>          exit }
>>      set rewritten flag
>>  }
>>  run relocator for jsr/ret - exit if exception
>>  link method entry points - exit if exception
>>
>> If there's an exception in relocator and linking method stages, these 
>> two actions are restartable if the VM handles the exception and has 
>> to retry loading that class.   The actions under the rewritten flag 
>> are not restartable.
>>
>> This is tested with invokedynamic tests, RedefineClasses tests and 
>> class data sharing.  There was special testing to throw OOM at each 
>> part and testing to rerun the relocator.
>>
>> This didn't make the deadline, but please review (again) for the next 
>> stage of the jdk7 release.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/7033141_2/
>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7033141_2
>>
>> Thanks,
>> Coleen
>>
>>
>> On 5/17/2011 4:44 PM, Coleen Phillimore wrote:
>>> Summary: Unrewrite bytecodes for OOM error allocating the constant 
>>> pool cache.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/7033141/
>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7033141
>>>
>>> This bug fix also moves linking methods outside of the rewriting 
>>> code so that when the code cache is full, the vm can attempt to link 
>>> the methods again.   (also fixes bug 6947901)
>>>
>>> Tested with oom errors inserted for random cpCache allocation and 
>>> also tested with invokedynamic test.
>>>
>>> This is somewhat urgent.  If acceptable, I would like to put it back 
>>> today for code freeze (if not, it'll wait).
>>>
>>> Thanks,
>>> Coleen
>>
>