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

[JC Beyler]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180720/5f7d53db/attachment-0001.html>