[9] RFR (XS): 8138922: StubCodeDesc constructor publishes partially-constructed objects on StubCodeDesc::_list

[Coleen Phillimore]

On 2/12/16 12:09 PM, Vladimir Ivanov wrote:
> Coleen,
>
>
>
>
> On 2/12/16 4:28 PM, Coleen Phillimore wrote:
>>
>> http://cr.openjdk.java.net/~vlivanov/8138922/webrev.01/src/share/vm/runtime/thread.cpp.udiff.html 
>>
>>
>>
>> This has a collision with
>>
>>   RFR: 8148630: Convert TraceStartupTime to Unified Logging
> Removed. I asked Rachel to cover java.lang.invoke case.

Good, thank you.  That makes it easy since your change and hers are 
going in different repositories.
>
>>
>> http://cr.openjdk.java.net/~vlivanov/8138922/webrev.01/src/share/vm/code/codeBlob.cpp.udiff.html 
>>
>>
>>
>> The 'new' operators already call vm_exit_out_of_memory rather than
>> returning null. MethodHandlesAdapterBlob may already do this.
> I don't see that happening in BufferBlob::operator new (which is 
> called in ). It allocates right in the code cache and returns NULL if 
> allocation fails.
>
> I replicate the code from other stub generators, e.g.:
> void StubRoutines::initialize2() {
> ...
>     _code2 = BufferBlob::create("StubRoutines (2)", code_size2);
>     if (_code2 == NULL) {
>       vm_exit_out_of_memory(code_size2, OOM_MALLOC_ERROR, "CodeCache: 
> no room for StubRoutines (2)");
>     }
>
> Or do you suggest to add MethodHandlesAdapterBlob::operator new and 
> move the check there?
>

No.  If the 'new' this ultimately calls doesn't already call 
vm_exit_out_of_memory, then you've done the right thing.

I don't see any other issues, but I don't really know this code so I'm a 
provisional review.

Coleen

> Updated webrev:
>   http://cr.openjdk.java.net/~vlivanov/8138922/webrev.02
>
> Had to move StubCodeDesc::freeze() call later in the init sequence: 
> JFR also allocates some stubs.
>
> Best regards,
> Vladimir Ivanov
>
>>
>> Coleen
>>
>> On 2/12/16 8:13 AM, Vladimir Ivanov wrote:
>>> Vladimir, David, Andrew, thanks again for the feedback.
>>>
>>> Updated version:
>>>   http://cr.openjdk.java.net/~vlivanov/8138922/webrev.01
>>>
>>> I moved method handle adapters generation to VM init phase and added
>>> verification logic to ensure there are no modifications to the
>>> StubCodeDesc::_list after that.
>>>
>>> Also, slightly refactored java.lang.invoke initialization logic.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 2/10/16 9:13 PM, Vladimir Ivanov wrote:
>>>> http://cr.openjdk.java.net/~vlivanov/8138922/webrev.00
>>>> https://bugs.openjdk.java.net/browse/JDK-8138922
>>>>
>>>> StubCodeDesc keeps a list of all descriptors rooted at
>>>> StubCodeDesc::_list by placing newly instantiated objects there at the
>>>> end of the constructor. Unfortunately, it doesn't guarantee that only
>>>> fully-constructed objects are visible, because compiler (or HW) can
>>>> reorder the stores.
>>>>
>>>> Since method handle adapters are generated on demand when j.l.i
>>>> framework is initialized, it's possible there are readers iterating 
>>>> over
>>>> the list at the moment. It's not a problem per se until everybody 
>>>> sees a
>>>> consistent view of the list.
>>>>
>>>> The fix is to insert a StoreStore barrier before registering an object
>>>> on the list.
>>>>
>>>> (I also considered moving MH adapter allocation to VM initialization
>>>> phase before anybody reads the list, but it's non-trivial since
>>>> MethodHandles::generate_adapters() has a number of implicit
>>>> dependencies.)
>>>>
>>>> Testing: manual (verified StubCodeMark assembly), JPRT
>>>>
>>>> Thanks!
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>