[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (T) 8243393: Improve ReservedSpace constructor resolution
From:       coleen.phillimore () oracle ! com
Date:       2020-04-29 13:05:29
Message-ID: 3544c1e5-f19d-89e1-8443-24b94e1de0e3 () oracle ! com
[Download RAW message or body]



On 4/29/20 9:00 AM, Thomas Stüfe wrote:
> I feel responsible because I opened the bug. I think that 
> ReservedSpace could do with some better commenting and possible 
> cleaning up, but I think that would not a five minutes project. Lets 
> fix the obvious bug and leave the clean up for later?

I'm looking at this now and I don't want to pull the string of this 
sweater any further.  The arguments to these constructors are unclear, 
ie: how preferred_alignment relates to the alignment passed in in 
constructor #2.

I tried to narrow this down to only one constructor because even with 
two they're easy to mess up, but that's a bigger project than my helping 
Thomas out with this RFE while I was researching something.

I'm just going to fix the change (which might get the same answer either 
way) for now.  Eventually the designation of "trivial" should be 
something that the compiler would warn you about but we're not there yet 
for this either.

Thanks,
Coleen

> 
> ..Thomas
> 
> On Wed, Apr 29, 2020 at 2:53 PM David Holmes <david.holmes@oracle.com 
> <mailto:david.holmes@oracle.com>> wrote:
> 
> On 29/04/2020 10:38 pm, coleen.phillimore@oracle.com
> <mailto:coleen.phillimore@oracle.com> wrote:
> > 
> > I already checked this in because it was trivial.
> 
> Didn't seem trivial to me.
> 
> > On 4/28/20 11:53 PM, David Holmes wrote:
> > > Hi Coleen,
> > > 
> > > On 28/04/2020 2:12 am, coleen.phillimore@oracle.com
> <mailto:coleen.phillimore@oracle.com> wrote:
> > > > Summary: Remove possibly ambiguous constructor and use
> directly in
> > > > ReservedCodeHeap
> > > > 
> > > > Tested with tier1-3.
> > > > 
> > > > open webrev at
> > > > http://cr.openjdk.java.net/~coleenp/2020/8243393.01/webrev
> > > > bug link https://bugs.openjdk.java.net/browse/JDK-8243393
> > > 
> > > Sorry but I don't understand the mapping to the constructors.
> In this
> > > original call from cardTable.cpp:
> > > 
> > > ReservedSpace heap_rs(_byte_map_size, rs_align, false);
> > > 
> > > this calls the 4-arg constructor as-if:
> > > 
> > > ReservedSpace heap_rs(_byte_map_size, rs_align, false, NULL);
> > > 
> > > and rs_align maps to the alignment parameter.
> > > 
> > > But in the new two-arg constructor call:
> > > 
> > > ReservedSpace heap_rs(_byte_map_size, rs_align)
> > > 
> > > the rs_align is no longer treated as alignment but rather
> > > preferred_page_size! It isn't at all obvious to me that this is an
> > > equivalent construction.
> > > 
> > 
> http://cr.openjdk.java.net/~coleenp/2020/8243393.01/webrev/src/hotspot/share/gc/shared/cardTable.cpp.udiff.html
>  
> > 
> > 
> > Yes, you're right, this is part of the change is wrong. It was
> matching
> > the version I took out.  I'll file a bug to fix it.
> 
> Okay.
> 
> > > Both constructors need to be adequately documented - at present
> only
> > > the two-arg constructor is.
> > 
> > Please suggest a comment.
> 
> Ha! I don't understand what the args to these constructors are
> supposed
> to indicate - that is the problem.
> 
> Cheers,
> David
> -----
> 
> > thanks,
> > Coleen
> > > 
> > > Thanks,
> > > David
> > > 
> http://cr.openjdk.java.net/~coleenp/2020/8243393.01/webrev/src/hotspot/share/gc/shared/cardTable.cpp.udiff.html
>  
> > > 
> > > > Thanks,
> > > > Coleen
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]