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

[Vladimir Kozlov]

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?

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

>
>> 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();

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

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