RFR: 8033764: Remove the usage of StarTask from BufferingOopClosure

[Mikael Gerdin]

Stefan,

I think it turned up pretty good, 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*?

  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?


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


(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".

/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