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

Mon Jul 23 03:04:15 UTC 2018

Thanks Igor!

http://cr.openjdk.java.net/~jcbeyler/8207252/webrev.04/

Has now your name in the reviewers. Would anyone want to push it by chance?

Thanks!
Jc

On Sat, Jul 21, 2018 at 7:39 PM Igor Veresov <igor.veresov at oracle.com>
wrote:

> 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>
> 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
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20180722/bc10e9ae/attachment-0001.html>