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

[Igor Ignatyev]

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
>>>>