[15] RFR: 8242895: failed: sanity at src/hotspot/share/opto/escape.cpp:2361
Jamsheed C M
jamsheed.c.m at oracle.com
Mon Jul 20 07:52:21 UTC 2020
Hi Vladimir,
Thank you for the review, I have updated the test
http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA.02/
Hi all,
Could I get another review ?
Best regards,
Jamsheed
On 18/07/2020 00:09, Vladimir Kozlov wrote:
> 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