RFR: 8212656: Clean up heap filler insertion

[Roman Kennke]

Hi Per,

> Hi Roman,
> 
> On 10/18/2018 03:57 PM, Roman Kennke wrote:
>> Hi Per,
>>
>> I'll try to weave that into Shenandoah and see how that goes :-)
>>
>> One thing that I don't see how it could work is how we could do extra
>> initialization for the fwd ptr? We can currently do it by overriding
>> fill_with_dummy_object(..) like this:
>>
>> void ShenandoahHeap::fill_with_dummy_object(HeapWord* start, HeapWord*
>> end, bool zap) {
>>    HeapWord* obj = post_allocation_setup(start); // Bumps start by 1 word
>>    CollectedHeap::fill_with_object(obj, end);
>> }
>>
>> This seems to have gone with your patch.
> 
> Ok, so if we do something like this:
> 
> --- a/src/hotspot/share/gc/shared/fill.cpp
> +++ b/src/hotspot/share/gc/shared/fill.cpp
> @@ -76,14 +76,14 @@
> 
>  void Fill::fill_with_object(HeapWord* start) {
>    const size_t size = object_header_size();
> -  ObjAllocator allocator(SystemDictionary::Object_klass(), size);
> -  allocator.initialize(start);
> +  ObjAllocator allocator(SystemDictionary::Object_klass(), size -
> _extra_header_size);
> +  allocator.initialize(start + _extra_header_size);
>  }
> 
>  void Fill::fill_with_array(HeapWord* start, size_t size) {
> -  const size_t length = array_length(size);
> -  ObjArrayAllocator allocator(Universe::intArrayKlassObj(), size,
> (int)length, false /* do_zero */);
> -  allocator.initialize(start);
> +  const size_t length = array_length(size - _extra_header_size);
> +  ObjArrayAllocator allocator(Universe::intArrayKlassObj(), size -
> _extra_header_size, (int)length, false /* do_zero */);
> +  allocator.initialize(start + _extra_header_size);
> 
>    if (ZapFillerObjects) {
>      HeapWord* const payload_start = start + array_header_size();
> 
> Then the object will be initialized in the correct position. You will
> not have the forwarding pointer initialized (we only made space for it
> to keep the layout intact), but I suspect that should not be needed for
> this purpose. The only reason this object exists is to make the heap
> parsable, so you should only be reading the klass pointer anyway, right?
> 

With the above changes, I can almost get it to work. I still need:

diff --git a/src/hotspot/share/gc/shared/fill.cpp
b/src/hotspot/share/gc/shared/fill.cpp
--- a/src/hotspot/share/gc/shared/fill.cpp
+++ b/src/hotspot/share/gc/shared/fill.cpp
@@ -47,7 +47,7 @@
 }

 size_t Fill::array_length(size_t size) {
-  return (size - array_header_size()) * (HeapWordSize / sizeof(jint));
+  return (size - typeArrayOopDesc::header_size(T_INT)) * (HeapWordSize
/ sizeof(jint));
 }

 void Fill::initialize() {


Otherwise the arraylength will be inconsistent.

You are right, very strictly speaking the fwd ptr field doesn't matter
there. However, it would still be nice to have a way to initialize that.
Our verifier would otherwise go crazy if it encounters a bad fwd ptr. :-)

Maybe it would be better to design the whole filler stuff with some
virtual methods, pretty much as it is now in CollectedHeap? This whole
business of _extra_words is already quite Shenandoah specific. We had a
similar concept earlier in our Shenandoah repo, where we'd have a
CH::extra_oop_words() method that would be used in allocation paths,
filler paths, object size paths, etc. But then I preferred to not have
this rather specific notion of extra words, plus the implicit idea that
the extra words *precede* the actual object in heap layout. This is all
implicit Shenandoah stuff. The way we have it now, this is completely
opaque in the interfaces and implementation. Can the Fill code be
designed like that instead?

Roman


> cheers,
> Per
> 
>>
>> Roman
>>
>>> While reviewing JDK-8211955, I suggested that the filler insertion code
>>> in CollectedHeap should be broken out into a Fill class (to match the
>>> somewhat related Copy class we already have). The filler insertion code
>>> doesn't have to live in the already crowded CollectedHeap class, as it's
>>> basically just a bunch of utility functions (just like the functions in
>>> the Copy class).
>>>
>>> With this patch CollectedHeap calls Fill::initialize() in it's
>>> constructor. This will set up the default behavior. Collectors with
>>> special needs (G1, ZGC and Shenandoah) can then call one or more of the
>>> Fill::set_*() functions to adjust filling behavior.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8212656
>>> Webrev: http://cr.openjdk.java.net/~pliden/8212656/webrev.0
>>>
>>> Testing: Passed tier{3,5,6} on all Oracle supported platforms, running
>>> additional tiers right now.
>>>
>>> /Per
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181018/5d7aad8b/signature.asc>