[15] RFR: 8242895: failed: sanity at src/hotspot/share/opto/escape.cpp:2361
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Jul 17 18:39:57 UTC 2020
Yes, I agree with webrev_fix_EA version.
I would suggest to modify TestIdealAllocShape.java test to add new method with synchronization from your example in JBS
comment. Or add it as separate test.
Thanks,
Vladimir
On 7/16/20 9:19 AM, Jamsheed C M wrote:
> Hi Vladimir,
> I ran performance run for http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA/ (links in JBS)
> I don't see any issues, so i would like to go with webrev_fix_EA if it fixes all the reported issues.
> Best regards,
> Jamsheed
>
> On 16/07/2020 07:25, Jamsheed C M wrote:
>> Hi Vladimir,
>>
>> On 16/07/2020 00:29, Vladimir Kozlov wrote:
>>> As I said before I agree with your additional checks for StoreN and StoreNKlass.
>>>
>>> But I have concerns about new is_init_captured_store code. EA is mostly looking only on inputs to see Allocation. And
>>> in several places it expecting only to see Allocation because other cases should be filtered out before.
>> If that is the case, I would like to go with my first webrev for this fix as it nicely propagate es and there in no
>> unnecessary promotion to global escape state.
>>
>> http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA/
>>
>> Best regards,
>>
>> Jamsheed
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 7/15/20 10:54 AM, Jamsheed C M wrote:
>>>> Hi Vladimir,
>>>>
>>>> with unrolling i understand that many cases will just have phis everywhere to outside the loop as the uses are
>>>> outside the loop.
>>>>
>>>> and this is not restricted to escaping objects alone as i depicted. it can be escaping as well as non-escaping.
>>>>
>>>> so marking store to them as global escape doesn't seems to be nice idea. i will rework on this fix and get back again.
>>>>
>>>> Thank you
>>>>
>>>> Best regards
>>>>
>>>> Jamsheed
>>>>
>>>> On 15/07/2020 08:38, Jamsheed C M wrote:
>>>>> (unfinished mail got sent, so completing it)
>>>>> On 15/07/2020 08:21, Jamsheed C M wrote:
>>>>>> Hi Vladimir,
>>>>>>
>>>>>> On 15/07/2020 06:50, Vladimir Kozlov wrote:
>>>>>>> I looked more on this. EA already does not secularize allocations when Phi nodes merged them - it should handle
>>>>>>> this case. I did small experiment and relaxed assert for this new (10. needs comment update) case for AddP's base
>>>>>>> and test passed:
>>>>>>>
>>>>>>> src/hotspot/share/opto/escape.cpp Tue Jul 14 18:11:27 2020 -0700
>>>>>>> @@ -2357,6 +2357,7 @@
>>>>>>> int opcode = uncast_base->Opcode();
>>>>>>> assert(opcode == Op_ConP || opcode == Op_ThreadLocal ||
>>>>>>> opcode == Op_CastX2P || uncast_base->is_DecodeNarrowPtr() ||
>>>>>>> + (uncast_base->is_Phi() && (uncast_base->bottom_type()->isa_rawptr() != NULL)) ||
>>>>>>> (uncast_base->is_Mem() && (uncast_base->bottom_type()->isa_rawptr() != NULL)) ||
>>>>>>> (uncast_base->is_Proj() && uncast_base->in(0)->is_Allocate()), "sanity");
>>>>>>> }
>>>>>>>
>>>>>>> Did you hit a case when this may not work?
>>>>>>
>>>>>> Yes, right it already doesn't mark it as scalarizable if base count is more than one(I think it missed a is_oop
>>>>>> check there)[1].
>>>>>>
>>>>>> EA CG adds edges only for oop field making stores to them undetected. This makes these stored objects to NoEscape
>>>>>> and if compiled method continues execution with this NoEscape object can have undesired results(i.e
>>>>>> synchronization removed).
>>>>>>
>>>>>> Probable case would be(didn't verify)
>>>>>>
>>>>>> try {
>>>>>>
>>>>>> LOOP BEGIN
>>>>>>
>>>>>> try {throw new Obj()} catch {}
>>>>>>
>>>>>> LOOP END
>>>>>>
>>>>>> } catch (Obj e) {
>>>>>>
>>>>>> }
>>>>>
>>>>> Best Regards,
>>>>>
>>>>> Jamsheed
>>>>>
>>>>> [1]https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/escape.cpp#L1770
>>>>>
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> And with LoopOpts off -XX:LoopUnrollLimit=0 it removed allocation (-XX:+PrintEscapeAnalysis
>>>>>>> -XX:+PrintEliminateAllocations):
>>>>>>>
>>>>>>> ======== Connection graph for Test::test
>>>>>>> JavaObject NoEscape(NoEscape) [ 158F [ 107 ]] 95 Allocate === 242 76 230 8 1 ( 93 92 21 1 78 1 78 ) [[
>>>>>>> 96 97 98 105 106 107 ]] rawptr:NotNull ( int:>=0, java/lang/Object:NotNull *, bool, top ) Test::test1 @ bci:0
>>>>>>> Test::test @ bci:8 !jvms: Test::test1 @ bci:0 Test::test @ bci:8
>>>>>>> LocalVar [ 95P [ 158b ]] 107 Proj === 95 [[ 108 158 ]] #5 !jvms: Test::test1 @ bci:0 Test::test @ bci:8
>>>>>>>
>>>>>>> Scalar 95 Allocate === 242 76 230 8 1 ( 93 92 21 1 78 1 78 ) [[ 96 97 98 105 106 107 ]]
>>>>>>> rawptr:NotNull ( int:>=0, java/lang/Object:NotNull *, bool, top ) Test::test1 @ bci:0 Test::test @ bci:8 !jvms:
>>>>>>> Test::test1 @ bci:0 Test::test @ bci:8
>>>>>>> ++++ Eliminated: 95 Allocate
>>>>>>>
>>>>>>>
>>>>>>> t\Thanks,
>>>>>>> Vladimir K
>>>>>>>
>>>>>>> On 7/14/20 1:28 AM, Jamsheed C M wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I had incorrectly added extra check in assert after offset computation in address_offset . For addps with non
>>>>>>>> constant offsets (like [1])
>>>>>>>>
>>>>>>>> Not changing the old assert even though I am not expecting first addp/second addp(for array addressing) case for
>>>>>>>> init captured store.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA_asserts_corrected/
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>> Jamsheed
>>>>>>>>
>>>>>>>> [1]
>>>>>>>>
>>>>>>>> assert(offs != Type::OffsetBot ||
>>>>>>>> - adr->in(AddPNode::Address)->in(0)->is_AllocateArray(),
>>>>>>>> + adr->in(AddPNode::Address)->in(0)->is_AllocateArray() || is_captured_store(adr),
>>>>>>>> "offset must be a constant or it is initialization of array");
>>>>>>>>
>>>>>>>> On 13/07/2020 11:14, Jamsheed C M wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I reworked the fix. I compute offset for all init captures stores, but treats this special init captured stores
>>>>>>>>> similar to unsafe(as these objects are usually GlobalEscape and doesn't have any perf implications).
>>>>>>>>>
>>>>>>>>> revised webrev: http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA.01/
>>>>>>>>>
>>>>>>>>> testing: mach1-5( logs in jbs)
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>>
>>>>>>>>> Jamsheed
>>>>>>>>>
>>>>>>>>> On 09/07/2020 19:36, Jamsheed C M wrote:
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> request to hold the review. need to change the code for dealing with unsafe access. as current capture code go
>>>>>>>>>> for more execution time analyzing things.
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>>
>>>>>>>>>> Jamsheed
>>>>>>>>>>
>>>>>>>>>> On 09/07/2020 13:01, Jamsheed C M wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8242895
>>>>>>>>>>>
>>>>>>>>>>> Request for review changes made to offset computation and field write detection for init captured stores due
>>>>>>>>>>> to phis addition between alloc and init. This happen if init node in different outer loop wrt to alloc node
>>>>>>>>>>> and there is a loop opt. This was required as a result of enhancement [1].
>>>>>>>>>>>
>>>>>>>>>>> Normally init are not associated with multiple alloc node during EA phase, but changes done for [1] caused
>>>>>>>>>>> the code shapes of the form [2] to generate inits associated with multiple alloc node.
>>>>>>>>>>>
>>>>>>>>>>> This had implication in offset computation and field write detection related to initializing stores.
>>>>>>>>>>>
>>>>>>>>>>> Attempt to fix in EA:
>>>>>>>>>>>
>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA/
>>>>>>>>>>>
>>>>>>>>>>> Alternate fix:
>>>>>>>>>>>
>>>>>>>>>>> Minimize the scenario in compiler generated code by throwing only j.l.Error from slowpath(all exception
>>>>>>>>>>> async/sync are handled in runtime exit).
>>>>>>>>>>>
>>>>>>>>>>> Stub epilog doesn't poll or throw any exceptions. Disable full loop opt before EA for detectable
>>>>>>>>>>> patterns and bailout EA for late detected patterns.
>>>>>>>>>>>
>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~jcm/8242895/webrev_deopt/
>>>>>>>>>>>
>>>>>>>>>>> Please advice.
>>>>>>>>>>>
>>>>>>>>>>> Testing : mach tier1-5 (logs in jbs)
>>>>>>>>>>>
>>>>>>>>>>> Best regards,
>>>>>>>>>>>
>>>>>>>>>>> Jamsheed
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [1] JDK-8231291 <https://bugs.openjdk.java.net/browse/JDK-8231291>C2: loop opts before EA should maximally
>>>>>>>>>>> unroll loops
>>>>>>>>>>>
>>>>>>>>>>> [2] that have its init node in different outer loop wrt to alloc node.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> loop begin
>>>>>>>>>>>
>>>>>>>>>>> try{
>>>>>>>>>>>
>>>>>>>>>>> return new obj()/ throw new obj()/ uncommon trap after allocation, in a loop
>>>>>>>>>>>
>>>>>>>>>>> } catch(ex) {
>>>>>>>>>>>
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> loop end
>>>>>>>>>>>
>>>>>>>>>>> 42 public static IntA test(int n) {
>>>>>>>>>>> 43 for (int i=0; i<2; i++) {
>>>>>>>>>>> 44 try {
>>>>>>>>>>> 45 return new IntA(n + i);
>>>>>>>>>>> 46 } catch (Exception e) {
>>>>>>>>>>> 47 }
>>>>>>>>>>> 48 }
>>>>>>>>>>> 49
>>>>>>>>>>>
More information about the hotspot-compiler-dev
mailing list