RFR: 8033764: Remove the usage of StarTask from BufferingOopClosure

[Bengt Rutisson]

Hi Stefan,

On 2014-02-06 16:58, Stefan Karlsson wrote:
> New webrevs:
>
> http://cr.openjdk.java.net/~stefank/8033764/webrev.01.delta/
> http://cr.openjdk.java.net/~stefank/8033764/webrev.01

Looks good to me.


>
> - Fixed most review comments. Waiting for more comments before 
> deciding on void* vs OopOrNarrowOopStar.

I don't have a strong opinion, but I kind of prefer the void* version.

Bengt

> - 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/20140210/31db6c34/attachment.htm>