RFR: 8351996: Behavioral updates for ClassValue::remove [v13]

[Viktor Klang]

On Sat, 10 May 2025 20:43:20 GMT, Chen Liang <liach at openjdk.org> wrote:

>> The recent patch #23866 makes calling `ClassValue::remove()` from `ClassValue::computeValue()` end up in infinite loops while fixing the stale value risk from the method.
>> 
>> The proposed fix is to preserve the stale value risk fix, and update the remove-from-compute behavior from the original designated no-op behavior to throwing an exception, as the original behavior conflicts with the stale value fix.
>> 
>> The implementation track the owner thread in promises (accessed in locked section); as a result, we can fail-fast on recursive removals from `computeValue`. I did not choose to use `ThreadTracker` as it is designed for single tracker and multiple threads, while this case here sees often just one thread, and the threads outlive the promise objects.
>> 
>> Also updated the API specs for `remove` to more concisely describe the memory effects. Please review the associated CSR as well.
>
> Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 22 additional commits since the last revision:
> 
>  - Jaikiran comments, improve thread reference tracking, more documents
>  - Merge branch 'master' of https://github.com/openjdk/jdk into fix/classvalue-compute-remove
>  - Update src/java.base/share/classes/java/lang/ClassValue.java
>    
>    Co-authored-by: Shaojin Wen <shaojin.wensj at alibaba-inc.com>
>  - Merge branch 'master' of https://github.com/openjdk/jdk into fix/classvalue-compute-remove
>  - Reviews from viktor
>  - Rewrite impl to follow the new simplified spec
>  - Merge branch 'master' of https://github.com/openjdk/jdk into fix/classvalue-compute-remove
>  - Try to simplify the model - use the finish of computeValue
>    
>  - Test updates - remove repetition, test case for no stale installation
>  - Fix incorrect promise removal when unnecessary and add regression test
>  - ... and 12 more: https://git.openjdk.org/jdk/compare/60bb1184...e24a1fd7

src/java.base/share/classes/java/lang/ClassValue.java line 99:

> 97:      * that computed the value to associate.  A new invocation to {@code
> 98:      * computeValue}, which that {@code remove} call happens-before, happens to
> 99:      * re-establish this happens-before relationship.</li>

Suggestion:

     * computeValue}, which that {@code remove} call happens-before,
     * re-establishes this happens-before relationship.</li>

src/java.base/share/classes/java/lang/ClassValue.java line 126:

> 124:      * value if it exists.  Otherwise, this method tries to associate a value
> 125:      * from a {@link #computeValue computeValue} invocation until the associated
> 126:      * value exists, which may be from another thread.

Suggestion:

     * value exists, which could be associated by a competing thread.

src/java.base/share/classes/java/lang/ClassValue.java line 347:

> 345:         }
> 346: 
> 347:         // Arguments are nullable, intentionally to allow initial tokens

Suggestion:

        // Arguments are intentionally nullable, to allow initial tokens

src/java.base/share/classes/java/lang/ClassValue.java line 348:

> 346: 
> 347:         // Arguments are nullable, intentionally to allow initial tokens
> 348:         static boolean areCompatible(RemovalToken current, RemovalToken original) {

Are "equivalent"?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2086738186
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2086742731
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2086745684
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2086747420