RFR: 8033764: Remove the usage of StarTask from BufferingOopClosure
Mikael Gerdin
mikael.gerdin at oracle.com
Thu Feb 6 16:40:51 UTC 2014
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
More information about the hotspot-gc-dev
mailing list