RFR(M) : 8059624 : Test task: WhiteBox API for testing segmented codecache feature

[Tobias Hartmann]

Hi Igor,

looks good (not a reviewer).

Best,
Tobias

On 08.11.2014 19:41, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev/8059624/webrev.02/
> 
> On 11/07/2014 04:25 AM, Vladimir Kozlov wrote:
>> On 11/6/14 10:02 AM, Igor Ignatyev wrote:
>>> updated webrev: http://cr.openjdk.java.net/~iignatyev/8059624/webrev.01/
>>> 679 lines changed: 643 ins; 2 del; 34 mod;
>>>
>>> Hi Vladimir/Tobias,
>>>
>>> thanks for review. please see my answers inline.
>>>
>>> On 11/06/2014 02:55 AM, Vladimir Kozlov wrote:
>>>> Hi Igor,
>>>>
>>>> codeBlob.hpp
>>>>
>>>> Why you need?:
>>>>
>>>> +   // placement
>>>> +   inline void* operator new(size_t s, void* p) throw() { return p; }
>>> I need this to execute ctor in WhiteBox::allocateCodeHeap:
>>>> 887   new (blob) BufferBlob("WB::DummyBlob", full_size);
>>>
>>> it's impossible to use default placement new, since BufferBlob overloads
>>> operator new.
>>
>> We do use ::new() in GrowableArray. Will it work in your case?
> y, it works, thanks.
>>
>>>
>>>> compileBroker.cpp
>>>>
>>>> I think MonitorLockerEx locker() should be before the loop. Then you may
>>>> not need to get lock in WB_UnlockCompilation.
>>> I moved MonitorLockerEx before the loop, but I still need a lock in
>>> UnlockCompilation to call notify_all();
>>
>> Should it be like next? Otherwise it looks like deadlock.
>>
>> + WB_ENTRY(void, WB_UnlockCompilation(JNIEnv* env, jobject o))
>> +   WhiteBox::compilation_locked = false;
>> +   {
>> +     MonitorLockerEx mo(Compilation_lock,
>> Mutex::_no_safepoint_check_flag);
>> +     mo.notify_all();
>> +   }
>> + WB_END
> there's no deadlock since Monitor::wait release the lock.
>>>
>>>> whitebox.cpp
>>>>
>>>> allocateCodeHeap() - what if CodeCache::allocate() failed to allocate?
>>> it will return null, see AllocationCodeHeapTest.java:
>>>>   93         while ((addr = WHITE_BOX.allocateCodeHeap(size, type.id))
>>>> != 0) {
>>>>   94             blobs.add(addr);
>>>>   95         }
>>
>> But in other cases you don't check:
>>
>>   73         long addr = WHITE_BOX.allocateCodeHeap(SIZE, type.id);
>>
>>   79         WHITE_BOX.freeCodeHeap(addr);
>>
>> And BufferBlob::free() does not check for NULL:
>>
>> void BufferBlob::free(BufferBlob *blob) {
>>    ThreadInVMfromUnknown __tiv;  // get to VM state in case we block on
>> CodeCache_lock
>>    blob->flush();
> I've added checks that addr isn't 0 to the tests.
>>>
>>>> In WB_GetCodeHeapEntries() checks that blobs array is not empty after
>>>> the loop which add entries.
>>> should I? I thought that blobs.length() will be 0 and blobs.begin() will
>>> equal blobs.end() in this case, won't it?
>>
>> Yes, but I am asking to add short-cut to bailout early to avoid
>> allocation of new array and other code which follows:
>>
>> +   ThreadToNativeFromVM ttn(thread);
>> +   jobjectArray result = NULL;
>> +   jclass clazz =
>> env->FindClass(vmSymbols::java_lang_Object()->as_C_string());
>> +   CHECK_JNI_EXCEPTION_(env, NULL);
>> +   result = env->NewObjectArray(blobs.length(), clazz, NULL);
> fixed
>>>
>>>> You need to prevent sweeping when you processing codecache blobs
>>>> (getCodeHeapEntries). Otherwise pointers from getCodeBlobs() may point
>>>> to dead or overwritten space, consider -Xcomp -XX:+DeoptimizeALot
>>> sweeper does its work after acquiring CodeCache_lock and there is lock
>>> CodeCache before 1st loop, so I introduced struct CodeBlobStub and store
>>> it in GrowableArray instead of CodeBlob.
>>
>> Yes, this works.
>>
>> Thanks,
>> Vladimir
>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>
>>> On 11/06/2014 12:11 PM, Tobias Hartmann wrote:
>>>> Hi Igor,
>>>>
>>>> looks good (not a reviewer). In addition to Vladimir's comments:
>>>>
>>>> 'WhiteBox::allocateCodeHeap' should be 'allocate_code_heap'. I think
>>>> the name is
>>>> a bit misleading since we don't allocate a CodeHeap but a CodeBlob in a
>>>> CodeHeap. Maybe use 'code_heap_allocate' or 'allocate_code_blob'. Same
>>>> for
>>>> 'WB_AllocateCodeHeap'.
>>> agree, changed it to AllocateCodeBlob/FreeCodeBlob
>>> // I just tried to mimic WB_AllocateMetaspace
>>>>
>>>> In 'BlobType::getAvailable' you always return the NonProfiledCodeHeap if
>>>> CodeCacheSegmentation is enabled. But if we only use the interpreter
>>>> (-Xint), we
>>>> don't have any method code heaps. Also with 'TieredStopAtLevel <=
>>>> CompLevel_simple' we don't need the ProfiledCodeHeap (see
>>>> 'CodeCache::heap_available'). The corresponding tests will hit the
>>>> "heap is
>>>> null" assert in 'CodeCache::allocate'.
>>> I updated 'BlobType::getAvailable' to handle such the situations.
>>>
>>>> On 11/5/14 8:25 AM, Igor Ignatyev wrote:
>>>>> http://cr.openjdk.java.net/~iignatyev/8059624/webrev.00/
>>>>> 660 lines changed: 624 ins; 2 del; 34 mod;
>>>>>
>>>>> Hi all,
>>>>>
>>>>> please review the patch which adds new WhiteBox methods needed for
>>>>> better testing SegmentedCodeCache:
>>>>>   - perform allocation in code cache
>>>>>   - force code cache sweep
>>>>>   - lock/unlock compilation
>>>>>   - get a segment id for nmethod.
>>>>> besides these methods, the patch also adds a method to get all entries
>>>>> in code heap.
>>>>>
>>>>> changes in product code:
>>>>>   - monitor 'Compilation_lock' was added to implement compilation
>>>>> locking/unlocking
>>>>>   - align_code_offset function was made a static member of CodeBlob
>>>>>   - WhiteBox was made a friend of several classes
>>>>>
>>>>> testing: jprt, new added tests
>>>>> jbs: https://bugs.openjdk.java.net/browse/JDK-8059624
>>>>>