RFR: 8228622: [lworld] Ineffective codegeneration for flattened arrays checks causes large performance regression on List iteration
Roland Westrelin
rwestrel at redhat.com
Fri Sep 20 15:32:19 UTC 2019
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