RFR (M): 8168467: Use TaskEntry as task mark queue elements

[Thomas Schatzl]

Hi Kim,

  sorry for the delay...

On Sat, 2017-02-25 at 14:06 -0500, Kim Barrett wrote:
> > 
> > On Feb 23, 2017, at 6:02 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> > 
> > Hi all,
> > 
> >   can I have reviews for this cleanup that changes the oop-misuse
> > for
> > marking (setting LSB to 1 for array slices) into adding a
> > proper G1TaskQueueEntry that hides that mess a little?
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8168467
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8168467/webrev/
> > Testing:
> > jprt, tier2+3 gc tests
> > 
> > Thanks,
> >   Thomas
> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>   77     return (HeapWord*)((uintptr_t)_holder &~ ArraySliceBit);
> 
> "&~ ArraySliceBit" => "& ~ArraySliceBit"

Fixed.

> 
> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>   42 #ifdef _MSC_VER
>   43 #pragma warning(push)
>   44 // warning C4522: multiple assignment operators specified
>   45 #pragma warning(disable:4522)
>   46 #endif
> 
> Is the non-volatile overload of operator= needed?

Yes, it is part of the API for the task queues.

> Or can the need for an assignment operator be eliminated?  That is,
> replace one or both with a named function, say "assign(...)".

What is the reason for this suggestion? Having two assignment
operators, one for volatile and other for non-volatile instances does
not seem to be really problematic. When I tried this the code doing the
assignments became more unnatural looking.

This seems to be more a case of the compiler being over-cautious than a
real problem.

I kept this for now.

> Note also that copy-assign w/o copy-construct is often considered a
> bad smell.

I added that one, although it is the same as the default one.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>   57   G1TaskQueueEntry(oop obj) : _holder(obj) { }
>   58   G1TaskQueueEntry(HeapWord* addr) :
> _holder((void*)((uintptr_t)addr | ArraySliceBit)) { }
> 
> Should these really be implicit conversions?

I think I fixed this by using factory methods and hidden constructors.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>   57   G1TaskQueueEntry(oop obj) : _holder(obj) { }
> 
> From usage elsewhere, I suspect obj must not be NULL.  If true, an
> assert would be good.

Fixed.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>  273   // Pushes the given buffer containing at most OopsPerChunk
> elements on the mark
> 
> OopsPerChunk is renamed.  There are other occurrences.

I did a lot of renaming in addition to that.

> 
> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>  278   bool par_push_chunk(G1TaskQueueEntry* buffer);
>  283   bool par_pop_chunk(G1TaskQueueEntry* buffer);
> 
> Pre-existing: I think the API might be cleaner if these took OopChunk
> arguments, rather than entry pointers.

I would prefer if that were done as part of JDK-8162952 because it
seems an obvious change to do in that context.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
> 2011   void operator()(G1TaskQueueEntry task_entry) const {
> 2012     guarantee(task_entry.is_array_slice() || task_entry.obj()-
> >is_oop(),
> ...
> 2015     guarantee(task_entry.is_array_slice() || !_g1h-
> >is_in_cset(task_entry.obj()),
> 
> Since these are guarantees, I think it would be more readable to
> hoist
> the is_array_slice check.

Fixed.

> 
> --------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
> 2467     G1TaskQueueEntry obj;
> ...
> 2470       scan_object(obj);
> 
> Shouldn't "obj" be "entry".  And scan_object seems misnamed.

Fixed. Also renamed other methods with "obj" getting entries.

> 
> ------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMarkObjArrayProcessor.hpp
>   53   // Process the given continuation "oop". Returns the number of
> words scanned.
>   54   size_t process_slice(HeapWord* slice);
> 
> Comment needs updating.

Fixed.

New webrev:
http://cr.openjdk.java.net/~tschatzl/8168467/webrev.0_to_1/ (incrementa
l)
http://cr.openjdk.java.net/~tschatzl/8168467/webrev.1/ (full)

Thanks,
  Thomas