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

Kim Barrett kim.barrett at oracle.com
Sat Feb 25 19:06:03 UTC 2017


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

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

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

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

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

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

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

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

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

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.cpp
2467     G1TaskQueueEntry obj;
...
2470       scan_object(obj);

Shouldn't "obj" be "entry".  And scan_object seems misnamed.

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

------------------------------------------------------------------------------ 



More information about the hotspot-gc-dev mailing list