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

Sat Jul 21 20:47:34 UTC 2018

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

