RFR (M): JDK-8035401: Fix visibility of G1ParScanThreadState members
Mikael Gerdin
mikael.gerdin at oracle.com
Thu Jun 26 12:13:25 UTC 2014
Bengt,
On Thursday 26 June 2014 13.43.17 Bengt Rutisson wrote:
> Hi Thomas,
>
> Looks good. One question though. In g1ParScanThreadState.hpp you have:
>
> 131 inline HeapWord* allocate(GCAllocPurpose purpose, size_t word_sz);
> 132 inline HeapWord* allocate_slow(GCAllocPurpose purpose, size_t
> word_sz);
> 133 inline void undo_allocation(GCAllocPurpose purpose, HeapWord*
> obj, size_t word_sz);
>
> But the methods are implemented in g1ParScanThreadState.cpp. Shouldn't
> the implementation be placed in g1ParScanThreadState.inline.hpp?
These are now private methods and are only ever called from the .cpp file so
they don't need to be visible outside it.
The inline keyword is sort of a hint to the compiler to inline them into the
caller, I'm not sure if it's strictly needed though.
I think the change in webrev.2 looks good.
/Mikael
>
> Thanks,
> Bengt
>
> On 2014-05-27 12:39, Thomas Schatzl wrote:
> > Hi Bengt,
> >
> > thanks for the review.
> >
> > On Wed, 2014-04-30 at 13:35 +0200, Bengt Rutisson wrote:
> >> Hi Thomas,
> >>
> >> Over all this looks good to me too.
> >>
> >> One question for g1ParScanThreadState.cpp. You have marked the
> >> deal_with_refrence() methods as "inline" even though they are in the
> >> same cpp file. Does that have any effect?
> >
> > I moved them to the inline files.
> >
> >> 394
> >> 395 template <class T> inline void
> >>
> >> G1ParScanThreadState::deal_with_reference(T* ref_to_scan) {
> >>
> >> 396 if (!has_partial_array_mask(ref_to_scan)) {
> >> 397 // Note: we can use "raw" versions of "region_containing"
> >> because
> >> 398 // "obj_to_scan" is definitely in the heap, and is not in a
> >> 399 // humongous region.
> >> 400 HeapRegion* r = _g1h->heap_region_containing_raw(ref_to_scan);
> >> 401 do_oop_evac(ref_to_scan, r);
> >> 402 } else {
> >> 403 do_oop_partial_array((oop*)ref_to_scan);
> >> 404 }
> >> 405 }
> >> 406
> >> 407 inline void G1ParScanThreadState::deal_with_reference(StarTask
> >> ref) {
> >> 408 assert(verify_task(ref), "sanity");
> >> 409 if (ref.is_narrow()) {
> >> 410 deal_with_reference((narrowOop*)ref);
> >> 411 } else {
> >> 412 deal_with_reference((oop*)ref);
> >> 413 }
> >> 414 }
> >>
> >> Also, I think that you have to declare methods that should be inlined
> >> before the place where they are being used on some platforms (Solaris).
> >> In this case I think it means that they should be declared before
> >> steal_and_trim_queue().
> >
> > Moved them to the inline file.
> >
> >> Personally I also find the new deal_with_reference(StarTask ref) a
> >> little confusing. With that method and the two methods generated by
> >> deal_with_reference(T* ref_to_scan) I get kind of unsure which method
> >>
> >> that will be executed by a call like:
> >> 156 StarTask stolen_task;
> >> 157 while (task_queues->steal(queue_num(), hash_seed(),
> >> stolen_task)) {
> >> 158 assert(verify_task(stolen_task), "sanity");
> >> 159 deal_with_reference(stolen_task);
> >>
> >> All three deal_with_reference() methods are potential matches. I assume
> >> the compiler prefers the deal_with_reference(StarTask ref) but it makes
> >> me unsure when I read the code.
> >
> > Changed to dispatch_reference().
> >
> >> One minor nit:
> >>
> >> g1ParScanThreadState.hpp
> >> You have changed the indentation of private/protected/public keywords to
> >> have one space indentation. That's fine as I think that is the standard,
> >> but since the whole file used no space indentation I would also have
> >> been fine with leaving that. However now the last "public" keyword is
> >> still having no space before it. Can you indent that too?
> >
> >> 218 public:
> > Fixed. Also removed superfluous newlines at the end of files.
> >
> > Also re-checked again for performance regressions, none found.
> >
> > Diff to last revision
> > http://cr.openjdk.java.net/~tschatzl/8035401/webrev.1_to_2/
> >
> > Full diff:
> > http://cr.openjdk.java.net/~tschatzl/8035401/webrev.2/
> >
> > (based on 8035400)
> >
> > Thanks,
> > Thomas
> >
> >> Thanks,
> >> Bengt
> >>
> >> On 2014-04-22 23:31, Jon Masamitsu wrote:
> >>> On 4/22/14 1:36 PM, Thomas Schatzl wrote:
> >>>> Hi Jon,
> >>>>
> >>>> On Tue, 2014-04-22 at 13:07 -0700, Jon Masamitsu wrote:
> >>>>> Thomas,
> >>>>>
> >>>>> What I see in these changes are
> >>>>>
> >>>>> 1) no semantic changes
> >>>>
> >>>> No.
> >>>>
> >>>>> 2) some methods in .hpp files moved to .cpp files
> >>>>
> >>>> Yes, because they were only referenced by the cpp file, so I thought it
> >>>> would be good to move them there. They will be inlined as needed anyway
> >>>> (and I think for some of them they were never inlined due to their
> >>>> size).
> >>>>
> >>>> I will do some more runs with the inline's added again.
> >>>>
> >>>>> 3) creation of steal_and_trim_queue() with definition in
> >>>>> a .cpp file (I may have missed additional such new
> >>>>> methods)
> >>>>
> >>>> There are none except queue_is_empty(), see below.
> >>>>
> >>>>> 4) change in visibility as the CR says
> >>>>
> >>>> That's the main change.
> >>>>
> >>>>> 5) no performance regressions as stated in your RFR
> >>>>
> >>>> No. Checked the results for the usual benchmarks (specjvm2008,
> >>>> specjbb05/2013) again right now, and there are no significant
> >>>> differences in the scores (on x64 and sparc), and for specjbb05/2013
> >>>> the
> >>>> average gc pause time, and the object copy time (assuming that this is
> >>>> the part that will be affected most) stay the same as in the baseline.
> >>>>
> >>>>> If that's what constitutes the change, looks good.
> >>>>
> >>>> Thanks.
> >>>>
> >>>>> Reviewed.
> >>>>>
> >>>>> If there is something more significant that I have
> >>>>> overlooked, please point me at it and I'll look again.
> >>>>
> >>>> There is not. Sorry, I should have pointed out the changes in more
> >>>> detail instead of you making guesses.
> >>>>
> >>>> Additional minor changes:
> >>>>
> >>>> - G1ParScanThreadState accesses members directly instead of using
> >>>> getters (e.g. _refs instead of refs()).
> >>>>
> >>>> - fixed some newlines in method declarations, removing newlines
> >>>>
> >>>> - removed refs() to avoid direct access from outside, and adding a new
> >>>> method queue_is_empty() (only used in asserts as refs()->is_empty(),
> >>>> and
> >>>> I did not want to expose refs() just for the asserts).
> >>>
> >>> All looks good.
> >>>
> >>> Reviewed.
> >>>
> >>> Jon
> >>>
> >>>> Thanks,
> >>>>
> >>>> Thomas
> >>>>>
> >>>>> On 4/18/14 6:29 AM, Thomas Schatzl wrote:
> >>>>>> Hi all,
> >>>>>>
> >>>>>> can I have reviews for this change? After moving
> >>>>>>
> >>>>>> G1ParScanThreadState,
> >>>>>> this change cleans up visibility, making a whole lot of stuff
> >>>>>> private.
> >>>>>>
> >>>>>> CR:
> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8035401
> >>>>>>
> >>>>>> Webrev:
> >>>>>> http://cr.openjdk.java.net/~tschatzl/8035401/webrev/
> >>>>>>
> >>>>>> Testing:
> >>>>>> perf testing indicated no changes, jprt
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>> Thomas
More information about the hotspot-gc-dev
mailing list