RFR: 8033764: Remove the usage of StarTask from BufferingOopClosure

[Stefan Karlsson]

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