Yeah, the fix I proposed doesn’t do exactly what we’d want. Sorry for the confusion. Your fix is fine. Reviewed.
igor > On Jul 21, 2018, at 7:06 PM, JC Beyler <jcbey...@google.com> wrote: > > Hi Igor, > > Thanks for looking at it! I don't know the code paths enough to know if that > is sufficient (I'll trust you evidently). I can run the tests next week if we > prefer that route. > > Were I to choose, I would prefer that interpreter/c1/c2 all follow the same > kind of paths, which would be my fix I believe: > 1) If TLAB, allocate there or slowpath > 2) Else If contiguous inline allocations are enabled, try that > 3) Goto Slowpath > > With your fix, even if we do not have the issue anymore, it still keeps code > that is not consistent but perhaps I'm missing something? > Jc > > On Sat, Jul 21, 2018 at 1:47 PM Igor Veresov <igor.vere...@oracle.com > <mailto:igor.vere...@oracle.com>> wrote: > I think you can just predicate the emission of these stubs for !UseTLAB, and > not mess with the CPU-specific code. What do you think? > > diff --git a/src/hotspot/share/c1/c1_LIRGenerator.cpp > b/src/hotspot/share/c1/c1_LIRGenerator.cpp > --- a/src/hotspot/share/c1/c1_LIRGenerator.cpp > +++ b/src/hotspot/share/c1/c1_LIRGenerator.cpp > @@ -674,7 +674,7 @@ > void LIRGenerator::new_instance(LIR_Opr dst, ciInstanceKlass* klass, bool > is_unresolved, LIR_Opr scratch1, LIR_Opr scratch2, LIR_Opr scratch3, LIR_Opr > scratch4, LIR_Opr klass_reg, CodeEmitInfo* info) { > klass2reg_with_patching(klass_reg, klass, info, is_unresolved); > // If klass is not loaded we do not know if the klass has finalizers: > - if (UseFastNewInstance && klass->is_loaded() > + if (UseFastNewInstance && !UseTLAB && klass->is_loaded() > && !Klass::layout_helper_needs_slow_path(klass->layout_helper())) { > > Runtime1::StubID stub_id = klass->is_initialized() ? > Runtime1::fast_new_instance_id : Runtime1::fast_new_instance_init_check_id; > > > igor > > > On Jul 20, 2018, at 12:37 PM, JC Beyler <jcbey...@google.com > > <mailto:jcbey...@google.com>> wrote: > > > > Yes that is right, this is the latest: > > http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.03/ > > <http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.03/> > > > > I apologize for the multiple threads and confusion, > > Jc > > > > On Fri, Jul 20, 2018 at 11:22 AM serguei.spit...@oracle.com > > <mailto:serguei.spit...@oracle.com> < > > serguei.spit...@oracle.com <mailto:serguei.spit...@oracle.com>> wrote: > > > >> Thank you a lot, Vladimir! > >> Yes, the webrev.03 is the latest. > >> Jc, will correct us if it is not right. > >> > >> Thanks, > >> Serguei > >> > >> > >> On 7/20/18 10:52, Vladimir Kozlov wrote: > >>> I asked Igor V. to look. > >>> > >>> Seems like review is done in an other thread which does not have bug > >>> id in subject. Currently webrev.03 > >>> > >>> Vladimir > >>> > >>> On 7/19/18 4:32 PM, serguei.spit...@oracle.com > >>> <mailto:serguei.spit...@oracle.com> wrote: > >>>> Thanks, Rahul! > >>>> In fact, there no good experts for this area in the serviceability team. > >>>> It would be much better if anyone from the Compiler team could do it. > >>>> > >>>> Vladimir K., > >>>> > >>>> Is there anyone from the Compiler team available to review this? > >>>> Otherwise, I could try to review it but am not sure about my review > >>>> quality. > >>>> > >>>> Thanks, > >>>> Serguei > >>>> > >>>> > >>>> On 7/19/18 00:48, Rahul Raghavan wrote: > >>>>> RFR (S) 8207252: C1 still does eden allocations when TLAB is enabled > >>>>> > >>>>> (just adding + hotspot-compiler-dev also) > >>>>> > >>>>> > >>>>> On Wednesday 18 July 2018 09:51 PM, JC Beyler wrote: > >>>>> Subject Was: > >>>>> Re: RFR (S): C1 still does eden allocations when TLAB is enabled > >>>>> > >>>>> + serviceability-dev > >>>>> > >>>>> Hi all, > >>>>> > >>>>> Could anyone else give me a review of this webrev and check/test the > >>>>> various architecture changes? > >>>>> > >>>>> http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.02/ > >>>>> <http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.02/> > >>>>> > >>>>> > >>>>> Thanks for all your help! > >>>>> Jc > >>>>> > >>>>> > >>>>>> On Mon, Jul 16, 2018 at 2:58 PM JC Beyler <jcbey...@google.com > >>>>>> <mailto:jcbey...@google.com>> > >> wrote: > >>>>>> > >>>>>>> Hi all, > >>>>>>> > >>>>>>> Here is a webrev that does all the architectures in the same way: > >>>>>>> http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.02/ > >>>>>>> <http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.02/> > >>>>>>> > >>>>>>> Could anyone review the other architectures and test? > >>>>>>> - arm, sparc & aarch64 are also modified now to follow the same > >>>>>>> "if no > >>>>>>> tlab, then consider eden space allocation" logic. > >>>>>>> > >>>>>>> Thanks for your help! > >>>>>>> Jc > >>>>>>> > >>>>>>> On Fri, Jul 13, 2018 at 9:16 PM JC Beyler <jcbey...@google.com > >>>>>>> <mailto:jcbey...@google.com>> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hi Kim, > >>>>>>>> > >>>>>>>> I opened this bug > >>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8190862 > >>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8190862> > >>>>>>>> > >>>>>>>> and now I've done an update: > >>>>>>>> http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.01/ > >>>>>>>> <http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.01/> > >>>>>>>> > >>>>>>>> I basically have done your nits but also removed the try_eden (it > >>>>>>>> was > >>>>>>>> used to bind a label but was not used). I updated the comments to > >>>>>>>> use the > >>>>>>>> one you preferred. > >>>>>>>> > >>>>>>>> I still have to do the other architectures though but at least we > >>>>>>>> seem to > >>>>>>>> have a consensus on this architecture, correct? > >>>>>>>> > >>>>>>>> Thanks for the review, > >>>>>>>> Jc > >>>>>>>> > >>>>>>>> On Fri, Jul 13, 2018 at 5:17 PM Kim Barrett <kim.barr...@oracle.com > >>>>>>>> <mailto:kim.barr...@oracle.com> > >>> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>>> On Jul 13, 2018, at 4:54 PM, JC Beyler <jcbey...@google.com > >>>>>>>>>> <mailto:jcbey...@google.com>> > >>>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>> Yes, you are right, I did those changes due to: > >>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8194084 > >>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8194084> > >>>>>>>>>> > >>>>>>>>>> If Robbin agrees to this change, and if no one sees an issue, > >>>>>>>>>> I'll go > >>>>>>>>> ahead > >>>>>>>>>> and propagate the change across architectures. > >>>>>>>>>> > >>>>>>>>>> Thanks for the review, I'll wait for Robbin (or anyone else's > >>>>>>>>>> comment > >>>>>>>>> and > >>>>>>>>>> review) :) > >>>>>>>>>> Jc > >>>>>>>>>> > >>>>>>>>>> On Fri, Jul 13, 2018 at 1:08 PM John Rose <john.r.r...@oracle.com > >>>>>>>>>> <mailto:john.r.r...@oracle.com> > >>> > >>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>>> On Jul 13, 2018, at 10:23 AM, JC Beyler <jcbey...@google.com > >>>>>>>>>>> <mailto:jcbey...@google.com>> > >>>>>>>>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> I'm not sure if we had left this case intentionally or not > >>>>>>>>>>> but, if we > >>>>>>>>> want > >>>>>>>>>>> it all to be consistent, we should perhaps fix it. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> Well, you put in that logic last February, so unless somebody > >>>>>>>>>>> speaks > >>>>>>>>> up > >>>>>>>>>>> quickly, I support your adjusting it to be the way you want it. > >>>>>>>>>>> > >>>>>>>>>>> Doing "hg grep -u supports_inline_contig_alloc -I > >>>>>>>>>>> src/hotspot/share" > >>>>>>>>>>> suggests that the GC group is most active in touching this > >>>>>>>>>>> feature. > >>>>>>>>>>> If Robbin is OK with it, there's your reviewer. > >>>>>>>>>>> > >>>>>>>>>>> FWIW, you can use me as a reviewer, but I'd get one other person > >>>>>>>>>>> working on the GC to OK it. > >>>>>>>>>>> > >>>>>>>>>>> — John > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> -- > >>>>>>>>>> > >>>>>>>>>> Thanks, > >>>>>>>>>> Jc > >>>>>>>>> > >>>>>>>>> Robbin is on vacation; you might not hear from him for a while. > >>>>>>>>> > >>>>>>>>> I'm assuming you'll open a new bug for this? > >>>>>>>>> > >>>>>>>>> Except for a few minor nits (below), this looks okay to me. > >>>>>>>>> > >>>>>>>>> The comment at line 1052 needs updating. > >>>>>>>>> > >>>>>>>>> pre-existing: The retry_tlab label declared on line 1054 is unused. > >>>>>>>>> > >>>>>>>>> pre-existing: The try_eden label declared on line 1054 is bound at > >>>>>>>>> line 1058, but unreferenced. > >>>>>>>>> > >>>>>>>>> I like the wording of the comment at 1139 better than the > >>>>>>>>> wording at > >>>>>>>>> 1016. > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>>> -- > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Jc > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> -- > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Jc > >>>>>>> > >>>>>> > >>>>>> > >>>> > >> > >> > > > > -- > > > > Thanks, > > Jc > > > > -- > > Thanks, > Jc
