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

[Vladimir Kozlov]

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