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

Sun Jul 22 02:06:26 UTC 2018

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>
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> wrote:
> >
> > Yes that is right, this is the latest:
> > 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 <
> > 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 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/
> >>>>>
> >>>>>
> >>>>> Thanks for all your help!
> >>>>> Jc
> >>>>>
> >>>>>
> >>>>>> On Mon, Jul 16, 2018 at 2:58 PM JC Beyler <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/
> >>>>>>>
> >>>>>>> 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>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Hi Kim,
> >>>>>>>>
> >>>>>>>> I opened this bug
> >>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8190862
> >>>>>>>>
> >>>>>>>> and now I've done an update:
> >>>>>>>> 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
> >>>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>>> On Jul 13, 2018, at 4:54 PM, JC Beyler <jcbeyler at google.com>
> >>>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Yes, you are right, I did those changes due to:
> >>>>>>>>>> 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
> >>>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> On Jul 13, 2018, at 10:23 AM, JC Beyler <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