RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code

[David Holmes]

Hi Ioi,

Thanks for taking a look.

On 4/02/2021 4:09 pm, Ioi Lam wrote:
> On Thu, 4 Feb 2021 05:25:15 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>> While examining changes related to the TRAPS macro I noticed that a number of CDS related methods were passed a thread parameter that ended up never being used. Fixing this percolates up the call chain resulting in the ability to remove the parameter from other calls. In some places the thread is needed and has to be manifested via Thread::current(), but these are few and non-critical, and the cleanup of the API (including the benefit to the TRAPS work - see JDK-8252685) makes this worthwhile.
>>
>> I also changed `Thread* THREAD` to TRAPS where it relates to exception processing (even if CDS diverts to an abort path).
>>
>> Some uses of CHECK were removed that were only passing THREAD and the called code never triggers any exceptions.
>>
>> Added commentary where it is not clear why we don't use CHECK.
>>
>> Testing: local build and CI tiers 1-3
>>
>> Thanks,
>> David
> 
> These changes look good to me.
> 
> You probably have found out that that some of the functions you changed (e.g.,  HeapShared::archive_reachable_objects_from) are executed inside the VMThread, which is not a JavaThread. So their current use of TRAPS/CHECK will not be compatible with your upcoming changes in JDK-8252685. The PR will fix these cases (but I am not sure if there are others).

Having TRAPS on such functions is plain wrong - if there were actually 
exceptions involved then the VMThread can't/shouldn't be executing the 
code. Yes I may find other cases where this problem exists.

> For the functions that you converted to use TRAPS, these are OK as they are called only inside a Java thread, before the CDS code switches to VM_PopulateDumpSharedSpace::doit().

Given their use of exceptions it is imperative they are only executed by 
JavaThreads. But thanks for confirming.

> ***
> 
> Just a side note ... the CDS code has more misuse of THREAD. But those can be done in a separate RFE, and they probably won't affect JDK-8252685.
> 
> E.g., MetaspaceShared::preload_and_dump should probably be changed to:
> 
> void MetaspaceShared::preload_and_dump(TRAPS) {
>      MetaspaceShared::preload_and_dump_impl(THREAD);
>      if (HAS_PENDING_EXCEPTION) {
>         log_error(cds)("CDS dumping has failed");
>         vm_exit(1);
>      }
> }
> 
> void MetaspaceShared::preload_and_dump_impl(TRAPS) {
>    .... the old MetaspaceShared::preload_and_dump()
>    .... change all THREAD to CHECK;
> }

Right - I didn't try to do a full audit, I simply found a particular 
case (that seemed big enough to warrant the effort) and pulled on that 
thread as far as I could. There may be other followup cleanups as I find 
more cases. :)

Thanks,
David

> -------------
> 
> Marked as reviewed by iklam (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/2397
>