[15] RFR: 8242895: failed: sanity at src/hotspot/share/opto/escape.cpp:2361
Jamsheed C M
jamsheed.c.m at oracle.com
Thu Jul 16 16:19:55 UTC 2020
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