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

[Vladimir Ivanov]

Vladimir, Coleen, David, thanks for reviews.

Best regards,
Vladimir Ivanov

On 2/12/16 10:28 PM, Vladimir Kozlov wrote:
> webrev.02 is fine for me.
>
> Thanks,
> Vladimir
>
> On 2/12/16 9:09 AM, 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.
>>
>>>
>>> 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?
>>
>> 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
>>>