|
Hi Jc,
I'll sponsor the push.
Thanks,
Serguei
On 7/22/18 20:04, JC Beyler wrote:
Thanks Igor!
Has now your name in the reviewers. Would anyone want to
push it by chance?
Thanks!
Jc
Yeah, the fix I proposed doesn’t do exactly what we’d
want. Sorry for the confusion. Your fix is fine. Reviewed.
igor
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
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
<jcbey...@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.spit...@oracle.com
<
> serguei.spit...@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.spit...@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 <jcbey...@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 <jcbey...@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.barr...@oracle.com
>>>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>> On Jul
13, 2018, at 4:54 PM, JC Beyler <jcbey...@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.r...@oracle.com
>>>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> On
Jul 13, 2018, at 10:23 AM, JC Beyler <jcbey...@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
--
--
|
