RFR: 8033764: Remove the usage of StarTask from BufferingOopClosure

[Mikael Gerdin]

On Thursday 06 February 2014 16.58.15 Stefan Karlsson wrote:
> 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 for generating the delta.
I switch/case is much better now and I agree with letting the next reviewer 
decide on the OopOrNarrowOopStar issue.

/Mikael

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