RFR: 8033764: Remove the usage of StarTask from BufferingOopClosure

[Stefan Karlsson]

New webrevs:

http://cr.openjdk.java.net/~stefank/8033764/webrev.01.delta/
http://cr.openjdk.java.net/~stefank/8033764/webrev.01

- Fixed most review comments. Waiting for more comments before deciding 
on void* vs OopOrNarrowOopStar.
- Fixed a copy-n-paste error in the tests:

-        case 1:  oops_do_narrow_then_full(cl);
+        case 1:
+          oops_do_full_then_narrow(cl);

thanks,
StefanK

On 06/02/14 14:43, Stefan Karlsson wrote:
> Mikael,
>
> On 06/02/14 14:12, Mikael Gerdin wrote:
>> Stefan,
>>
>> I think it turned up pretty good,
>
> Thanks.
>
>>   I have some primarily stylistic comments
>> inline.
>>
>> On Thursday 06 February 2014 12.20.34 Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> please review the following patch:
>>> http://cr.openjdk.java.net/~stefank/8033764/webrev.00/
>> -> bufferingOopClosure.hpp:
>>
>>    50   void*  _buffer[BufferLength];
>>    51   void** _oop_top;
>>    52   void** _narrowOop_bottom;
>> Would it make sense to use the typedef OopOrNarrowOopStar (or a 
>> union) instead
>> of raw void*?
>
> I don't mind the raw void*. I'll change it to OopOrNarrowOopStar if 
> the second reviewer agrees with you.
>
>>
>>    62   bool is_buffer_full() {
>>    63     return (uintptr_t)_narrowOop_bottom < (uintptr_t)_oop_top;
>>    64   }
>> Why do you cast the pointers to uintptr_t before the compare?
>
> I'll remove the cast.
>
>>
>>
>> -> bufferingOopClosure.cpp
>>
>>   141
>>   142   static void testCount(int num_narrow, int num_full, int 
>> do_oop_order) {
>>   143     FakeRoots fr(num_narrow, num_full);
>>   144
>> Can you please add the parameter combination which caused the assert 
>> to fail
>> to simplify investigating test failures?
>>
>> If you don't want to do all the printing in the test cases you can 
>> use macro
>> like execute_internal_vm_tests does:
>> 5053 #define run_unit_test(unit_test_function_call) \
>> 5054   tty->print_cr("Running test: " #unit_test_function_call); \
>> 5055   unit_test_function_call
>
> Sure. Let's see what I end up with.
>
>>
>>
>> (nitpicking)
>>   100     void oops_do(OopClosure* cl, int do_oop_order) {
>>   101       switch(do_oop_order) {
>>   102         case 0:  oops_do_narrow_then_full(cl);
>>   103         break;
>> I'm pretty sure that we usually indent the break one level past the 
>> "case".
> I'll change it.
>
> Thanks for reviewing!
>
> StefanK
>
>>
>> /Mikael
>>
>>> for the RFE:
>>> https://bugs.openjdk.java.net/browse/JDK-8033764
>>>
>>> This is a first step to simplify/unify the code root scanning, 
>>> something
>>> we want for the G1 Class Unloading project:
>>> http://openjdk.java.net/jeps/156
>>>
>>> Background:
>>>
>>> G1 uses the BufferingOopClosure to separate the root iteration from the
>>> object copying. The oop*/narrowOop* roots are gathered in a buffer and
>>> then bulk processed. By only timing the processing part and not the 
>>> root
>>> iteration, the object copying time can be measured.
>>>
>>> Currently, the BufferingOopClosure uses StarTask, from the taskqueue
>>> code, to differentiate between oop* and narrowOop* roots. The StarTask
>>> uses the least significant bit to mark whether the address contains an
>>> oop or a narrowOop. This works for most of the roots, since the
>>> addresses are aligned and the LSB is always 0. However, oops can be
>>> embedded as immediates in the JITed code and the addresses for these 
>>> are
>>> not necessarily aligned. This prevents us from using StarTasks with 
>>> oops
>>> in the CodeCache.
>>>
>>> See this comment from g1_process_strong_roots:
>>> ...
>>> BufferingOopClosure buf_scan_non_heap_roots(scan_non_heap_roots);
>>>
>>> // Walk the code cache/strong code roots w/o buffering, because 
>>> StarTask
>>> // cannot handle unaligned oop locations.
>>> CodeBlobToOopClosure eager_scan_code_roots(scan_non_heap_roots, true /*
>>> do_marking */);
>>> ...
>>>
>>> I suggest that we replace the StartTask usage with another
>>> implementation that allows the BufferingOopClosures to be used for the
>>> CodeCache scanning.
>>>
>>> thanks,
>>> StefanK
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140206/009d2ad1/attachment.htm>