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