RFR: 8228622: [lworld] Ineffective codegeneration for flattened arrays checks causes large performance regression on List iteration
Tobias Hartmann
tobias.hartmann at oracle.com
Mon Sep 23 06:44:45 UTC 2019
Hi Roland,
thanks for the explanations and changes. Looks good to me.
However, the latest webrev requires merging due to JDK-8225653:
--- a/src/hotspot/share/opto/callnode.cpp Mon Sep 23 07:21:54 2019 +0200
+++ b/src/hotspot/share/opto/callnode.cpp Mon Sep 23 07:52:08 2019 +0200
@@ -1237,7 +1237,7 @@
igvn->register_new_node_with_optimizer(unc);
Node* ctrl = phase->transform(new ProjNode(unc, TypeFunc::Control));
- Node* halt = phase->transform(new HaltNode(ctrl, alloc->in(TypeFunc::FramePtr)));
+ Node* halt = phase->transform(new HaltNode(ctrl, alloc->in(TypeFunc::FramePtr), "uncommon trap
returned which should never happen"));
phase->C->root()->add_req(halt);
return true;
And unfortunately, TestLWorld.test93 fails:
Caused by: java.lang.RuntimeException: assertTrue: expected true, was false
at jdk.test.lib.Asserts.fail(Asserts.java:594)
at jdk.test.lib.Asserts.assertTrue(Asserts.java:486)
at jdk.test.lib.Asserts.assertTrue(Asserts.java:472)
at compiler.valhalla.valuetypes.TestLWorld.test93_verifier(TestLWorld.java:2300)
... 6 more
Flags are: -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server
-XX:-TieredCompilation
Best regards,
Tobias
On 20.09.19 17:32, Roland Westrelin wrote:
>
> Hi Tobias,
>
> Thanks for the review.
>
>> All tests pass now and I've only found some minor things:
>> - Should TypeInstPtr::_flatten_array not be TypeInstPtr::_flat_array or _flattened_array?
>
> Right.
>
>> - memnode.cpp:2360: Missing whitespace before {"
>
> Ok.
>
>> - parse2.cpp:85 and 277: Why are you not simply comparing 'flattened' to zero? Is it because it
>> might be int/long depending on compressed klass ptrs?
>
> Is the question why it's a ne comparison and not eq?
> The condition will be canonicalized to ne once IGVN runs. So to have
> IfNode::is_flattened_array_check() work at parse time and igvn, it was
> simpler to switch to ne.
>
>> - phaseX.cpp:1695: Please add a comment describing why that code is
>> needed.
>
> Ok.
>
>> - type.cpp:4127 'value' -> 'flatten_array'?
>
> Ok.
>
>> - TestLWorld.java: Shouldn't that test verify that an uncommon trap was added and that we don't have
>> a LOAD_UNKNOWN_VALUE match?
>
> Yes. Except this test causes a recompilation and the verification code
> checks the recompiled version where there's a LOAD_UNKNOWN_VALUE. So I
> made a copy of the test. I also added a new one. Revisiting the tests, I
> realized they don't test the right thing: on a simple test, c2 doesn't
> run enough loop opts pass so the optimization doesn't have a chance to
> run. I had to tweak the tests. I also adjusted in
> CallStaticJavaNode::remove_useless_allocation() so test94 optimizes
> correctly.
>
>> - ValueTypeTest.java already defines the compilation levels (see line 110)
>
> Ok.
>
>> No need to send a new webrev.
>
> Here is a new one anyway.
>
> http://cr.openjdk.java.net/~roland/8228622/webrev.03/
>
> Roland.
>
More information about the valhalla-dev
mailing list