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

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(L) 8223320: [AOT] jck test api/javax_script/ScriptEngine/PutGet.html fails when test classes
From:       Igor Veresov <igor.veresov () oracle ! com>
Date:       2019-05-31 19:50:41
Message-ID: 95A23A32-4C0D-438F-A13A-D59938D88330 () oracle ! com
[Download RAW message or body]

Thanks Vladimir and Tom!


igor



> On May 31, 2019, at 12:36 PM, Tom Rodriguez <tom.rodriguez@oracle.com> wrote:
> 
> Looks good.
> 
> tom
> 
> Igor Veresov wrote on 5/31/19 12:30 PM:
> > Thanks for review!
> > I did the renames and added support for ByteCache.
> > Updated webrev: http://cr.openjdk.java.net/~iveresov/8223320/webrev.02/ \
> > <http://cr.openjdk.java.net/%7Eiveresov/8223320/webrev.02/> igor
> > > On May 31, 2019, at 11:36 AM, Tom Rodriguez <tom.rodriguez@oracle.com \
> > > <mailto:tom.rodriguez@oracle.com>> wrote: 
> > > you're missing the ByteCache.
> > > 
> > > Isn't box the generic term for these classes whether they are cached or not?  I \
> > > guess what's confusing is that both new Integer(n) and Integer.valueOf(n) are \
> > > both boxes but one with be marked as box and the other won't.  It might be \
> > > clearer to use cached or auto_box instead of just box. 
> > > Otherwise this looks ok to me.  Thanks for taking care of this.
> > > 
> > > tom
> > > 
> > > Igor Veresov wrote on 5/30/19 3:29 PM:
> > > > > On May 30, 2019, at 2:15 PM, Vladimir Kozlov <vladimir.kozlov@oracle.com \
> > > > > <mailto:vladimir.kozlov@oracle.com> <mailto:vladimir.kozlov@oracle.com>> \
> > > > > wrote: 
> > > > > CCing to runtime group too.
> > > > > 
> > > > > So you went hard and correct way ;-)
> > > > Well, there isn't much of choice… Must abide the spec.
> > > > > 
> > > > > deoptimization.cpp next #ifdef sequence is strange since we support only \
> > > > > 64-bit on SPARC: 
> > > > > +#ifdef _LP64
> > > > > +                       jlong res = (jlong)low->get_int();
> > > > > +#else
> > > > > +#ifdef SPARC
> > > > > 
> > > > Right. I guess JVMCI and AOT will never support 32 bit, so I'll just remove \
> > > > it.
> > > > > Also the code here is guarded by INCLUDE_JVMCI but it should be applicable \
> > > > > to AOT code too. Right? May be || INCLUDE_AOT? 
> > > > Good point, will add that.
> > > > > Please, add more comment. For example add one in aotLoader.cpp for \
> > > > > initialize_box_caches() to explain why we need to eager initialize caches \
> > > > > for AOT. 
> > > > Ok.
> > > > New webrev: http://cr.openjdk.java.net/~iveresov/8223320/webrev.01/ \
> > > > <http://cr.openjdk.java.net/%7Eiveresov/8223320/webrev.01/> \
> > > > <http://cr.openjdk.java.net/%7Eiveresov/8223320/webrev.01/> igor
> > > > > Thanks,
> > > > > Vladimir
> > > > > 
> > > > > On 5/30/19 12:33 PM, Igor Veresov wrote:
> > > > > > Graal models boxing (a call to valueOf()) as a BoxNode. If scalarized, it \
> > > > > > is encoded in the debug info as an allocation of a box object. However, \
> > > > > > for certain ranges of values the box object has to come from caches. The \
> > > > > > reason is that for these values JLS guarantees the identity of the boxes. \
> > > > > > The fix essentially propagates the information on whether the Box is a \
> > > > > > result of Box.valueOf() or new Box() to the deoptimization machinery that \
> > > > > > checks if the object is in the range that should be in a cache and gets \
> > > > > >                 it from there instead of allocating it.
> > > > > > Mach5: tier1-6, tier2-6 with Graal
> > > > > > Webrev: http://cr.openjdk.java.net/~iveresov/8223320/webrev.00/ \
> > > > > > <http://cr.openjdk.java.net/%7Eiveresov/8223320/webrev.00/> \
> > > > > > <http://cr.openjdk.java.net/%7Eiveresov/8223320/webrev.00/> I'd like to \
> > > > > > push all this into JDK13 first and then follow up with a change to the \
> > > > > > upstream Graal. Thanks,
> > > > > > igor


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