RFR (S) 8207252: C1 still does eden allocations when TLAB is enabled

Igor Veresov igor.veresov at oracle.com
Sun Jul 22 02:39:21 UTC 2018 

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 <jcbeyler at 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.veresov at oracle.com <mailto:igor.veresov at 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 <jcbeyler at google.com <mailto:jcbeyler at 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.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> <
> > serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at 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.spitsyn at oracle.com <mailto:serguei.spitsyn at 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 <jcbeyler at google.com <mailto:jcbeyler at 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 <jcbeyler at google.com <mailto:jcbeyler at 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.barrett at oracle.com <mailto:kim.barrett at oracle.com>
> >>> 
> >>>>>>>> wrote:
> >>>>>>>> 
> >>>>>>>>>> On Jul 13, 2018, at 4:54 PM, JC Beyler <jcbeyler at google.com <mailto:jcbeyler at 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.rose at oracle.com <mailto:john.r.rose at oracle.com>
> >>> 
> >>>>>>>>> wrote:
> >>>>>>>>>> 
> >>>>>>>>>>> On Jul 13, 2018, at 10:23 AM, JC Beyler <jcbeyler at google.com <mailto:jcbeyler at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20180721/3d7aab10/attachment-0001.html>

More information about the hotspot-compiler-dev mailing list