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

[serguei.spitsyn at oracle.com]

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