RFR: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap

Hui Shi-2
Add HandleMark in Deoptimization::uncommon_trap before Deoptimization::fetch_unroll_info_helper, avoid reference hold in HandleArea increase object lifetime. Then object lifetime will be consistent with/without uncommon trap.

For test case in commit, WeakReference is expected cleared after GC, but it fails with option "-XX:-Inline -XX:-TieredCompilation -XX:CompileCommand=compileonly,UncommonTrapLeak.foo -XX:CompileThreshold=100 -XX:-BackgroundCompilation". Reference's referent object is still alive after "foo" finish, because with uncommon trap, oops are recorded in HandleArea and HandleArea is not poped when uncommon trap process finish.

When Deoptimization::fetch_unroll_info_helper return, all oops in deoptimized frames are saved in Deoptimization::UnrollBlock or Thread data structure, HandleArea can be poped safely.
1. local and expression oops raw address is stored in vframeArrayElement _locals/_expressions as intptr
2. return value restore, raw oop recoreded in frame // (oop *)map->location(rax->as_VMReg());
3. exception object, raw oop recorded on Thread._exception_oop

In deoptimize blob entry, JRT_BLOCK_ENTRY(Deoptimization::fetch_unroll_info) has HandleMarkCleaner, HandleArea is restored after Deoptimization::fetch_unroll_info_helper finish. So it's also safe to add HandleMark in Deoptimization::uncommon_trap before fetch_unroll_info_helper.

-------------

Commit messages:
 - 8261585: Restore HandleArea used in Deoptimization::uncommon_trap

Changes: https://git.openjdk.java.net/jdk/pull/2526/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2526&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261585
  Stats: 64 lines in 2 files changed: 64 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2526.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2526/head:pull/2526

PR: https://git.openjdk.java.net/jdk/pull/2526
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap

Coleen Phillimore-3
On Thu, 11 Feb 2021 14:42:59 GMT, Hui Shi <[hidden email]> wrote:

> Add HandleMark in Deoptimization::uncommon_trap before Deoptimization::fetch_unroll_info_helper, avoid reference hold in HandleArea increase object lifetime. Then object lifetime will be consistent with/without uncommon trap.
>
> For test case in commit, WeakReference is expected cleared after GC, but it fails with option "-XX:-Inline -XX:-TieredCompilation -XX:CompileCommand=compileonly,UncommonTrapLeak.foo -XX:CompileThreshold=100 -XX:-BackgroundCompilation". Reference's referent object is still alive after "foo" finish, because with uncommon trap, oops are recorded in HandleArea and HandleArea is not poped when uncommon trap process finish.
>
> When Deoptimization::fetch_unroll_info_helper return, all oops in deoptimized frames are saved in Deoptimization::UnrollBlock or Thread data structure, HandleArea can be poped safely.
> 1. local and expression oops raw address is stored in vframeArrayElement _locals/_expressions as intptr
> 2. return value restore, raw oop recoreded in frame // (oop *)map->location(rax->as_VMReg());
> 3. exception object, raw oop recorded on Thread._exception_oop
>
> In deoptimize blob entry, JRT_BLOCK_ENTRY(Deoptimization::fetch_unroll_info) has HandleMarkCleaner, HandleArea is restored after Deoptimization::fetch_unroll_info_helper finish. So it's also safe to add HandleMark in Deoptimization::uncommon_trap before fetch_unroll_info_helper.

Marked as reviewed by coleenp (Reviewer).

src/hotspot/share/runtime/deoptimization.cpp line 2468:

> 2466:     uncommon_trap_inner(thread, trap_request);
> 2467:   }
> 2468:   HandleMark hm(thread);

This is fine. I generally prefer the HandleMark closer to the lifetimes of the Handles that it protects.  If you put this HandleMark in fetch_unroll_info_helper() would it be needed before the line:
  Handle exceptionObject;
Or are there Handles created before there?
Given how complicated this code is, where you have the HandleMark is fine.  Well done finding this bug and writing a test for it!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2526
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap

Hui Shi-2
On Thu, 11 Feb 2021 18:10:35 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Add HandleMark in Deoptimization::uncommon_trap before Deoptimization::fetch_unroll_info_helper, avoid reference hold in HandleArea increase object lifetime. Then object lifetime will be consistent with/without uncommon trap.
>>
>> For test case in commit, WeakReference is expected cleared after GC, but it fails with option "-XX:-Inline -XX:-TieredCompilation -XX:CompileCommand=compileonly,UncommonTrapLeak.foo -XX:CompileThreshold=100 -XX:-BackgroundCompilation". Reference's referent object is still alive after "foo" finish, because with uncommon trap, oops are recorded in HandleArea and HandleArea is not poped when uncommon trap process finish.
>>
>> When Deoptimization::fetch_unroll_info_helper return, all oops in deoptimized frames are saved in Deoptimization::UnrollBlock or Thread data structure, HandleArea can be poped safely.
>> 1. local and expression oops raw address is stored in vframeArrayElement _locals/_expressions as intptr
>> 2. return value restore, raw oop recoreded in frame // (oop *)map->location(rax->as_VMReg());
>> 3. exception object, raw oop recorded on Thread._exception_oop
>>
>> In deoptimize blob entry, JRT_BLOCK_ENTRY(Deoptimization::fetch_unroll_info) has HandleMarkCleaner, HandleArea is restored after Deoptimization::fetch_unroll_info_helper finish. So it's also safe to add HandleMark in Deoptimization::uncommon_trap before fetch_unroll_info_helper.
>
> src/hotspot/share/runtime/deoptimization.cpp line 2468:
>
>> 2466:     uncommon_trap_inner(thread, trap_request);
>> 2467:   }
>> 2468:   HandleMark hm(thread);
>
> This is fine. I generally prefer the HandleMark closer to the lifetimes of the Handles that it protects.  If you put this HandleMark in fetch_unroll_info_helper() would it be needed before the line:
>   Handle exceptionObject;
> Or are there Handles created before there?
> Given how complicated this code is, where you have the HandleMark is fine.  Well done finding this bug and writing a test for it!

Thanks!

There are also Handles used in static method eliminate_allocations and eliminate_locks.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2526
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap

Jie Fu-2
In reply to this post by Hui Shi-2
On Thu, 11 Feb 2021 14:42:59 GMT, Hui Shi <[hidden email]> wrote:

> Add HandleMark in Deoptimization::uncommon_trap before Deoptimization::fetch_unroll_info_helper, avoid reference hold in HandleArea increase object lifetime. Then object lifetime will be consistent with/without uncommon trap.
>
> For test case in commit, WeakReference is expected cleared after GC, but it fails with option "-XX:-Inline -XX:-TieredCompilation -XX:CompileCommand=compileonly,UncommonTrapLeak.foo -XX:CompileThreshold=100 -XX:-BackgroundCompilation". Reference's referent object is still alive after "foo" finish, because with uncommon trap, oops are recorded in HandleArea and HandleArea is not poped when uncommon trap process finish.
>
> When Deoptimization::fetch_unroll_info_helper return, all oops in deoptimized frames are saved in Deoptimization::UnrollBlock or Thread data structure, HandleArea can be poped safely.
> 1. local and expression oops raw address is stored in vframeArrayElement _locals/_expressions as intptr
> 2. return value restore, raw oop recoreded in frame // (oop *)map->location(rax->as_VMReg());
> 3. exception object, raw oop recorded on Thread._exception_oop
>
> In deoptimize blob entry, JRT_BLOCK_ENTRY(Deoptimization::fetch_unroll_info) has HandleMarkCleaner, HandleArea is restored after Deoptimization::fetch_unroll_info_helper finish. So it's also safe to add HandleMark in Deoptimization::uncommon_trap before fetch_unroll_info_helper.

Looks good to me.
Thanks for fixing it.

I'd like to sponsor it.
Thanks.

-------------

Marked as reviewed by jiefu (Committer).

PR: https://git.openjdk.java.net/jdk/pull/2526
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap

Hui Shi-2
In reply to this post by Hui Shi-2
On Thu, 11 Feb 2021 14:42:59 GMT, Hui Shi <[hidden email]> wrote:

> Add HandleMark in Deoptimization::uncommon_trap before Deoptimization::fetch_unroll_info_helper, avoid reference hold in HandleArea increase object lifetime. Then object lifetime will be consistent with/without uncommon trap.
>
> For test case in commit, WeakReference is expected cleared after GC, but it fails with option "-XX:-Inline -XX:-TieredCompilation -XX:CompileCommand=compileonly,UncommonTrapLeak.foo -XX:CompileThreshold=100 -XX:-BackgroundCompilation". Reference's referent object is still alive after "foo" finish, because with uncommon trap, oops are recorded in HandleArea and HandleArea is not poped when uncommon trap process finish.
>
> When Deoptimization::fetch_unroll_info_helper return, all oops in deoptimized frames are saved in Deoptimization::UnrollBlock or Thread data structure, HandleArea can be poped safely.
> 1. local and expression oops raw address is stored in vframeArrayElement _locals/_expressions as intptr
> 2. return value restore, raw oop recoreded in frame // (oop *)map->location(rax->as_VMReg());
> 3. exception object, raw oop recorded on Thread._exception_oop
>
> In deoptimize blob entry, JRT_BLOCK_ENTRY(Deoptimization::fetch_unroll_info) has HandleMarkCleaner, HandleArea is restored after Deoptimization::fetch_unroll_info_helper finish. So it's also safe to add HandleMark in Deoptimization::uncommon_trap before fetch_unroll_info_helper.

This pull request has now been integrated.

Changeset: 95d73129
Author:    Hui Shi <[hidden email]>
Committer: Jie Fu <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/95d73129
Stats:     64 lines in 2 files changed: 64 ins; 0 del; 0 mod

8261585: Restore HandleArea used in Deoptimization::uncommon_trap

Reviewed-by: coleenp, jiefu

-------------

PR: https://git.openjdk.java.net/jdk/pull/2526