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

[Viktor Klang]

On Tue, 29 Apr 2025 19:03:08 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 17 additional commits since the last revision:
> 
>  - 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
>  - Cannot test for recursion eagerly - add test case
>  - More spec, eager exception, finish with existing, thanks John
>  - Merge branch 'master' of https://github.com/openjdk/jdk into fix/classvalue-compute-remove
>  - docs
>  - Remove the throwing behavior due to shallow reentrancy
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/56c8a5ac...221d51e7

Hi @liach, I had a look, primarily on the changes rather than the tests, for this pass.

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

> 93:      * <ul>
> 94:      * <li>This association is already associated by another attempt. The
> 95:      * associated value is returned.</li>

It's a bit unclear which association is referred to in the second sentence, I think that could be made clearer with something like:

Suggestion:

     * <li>This association is already associated by another attempt, so that value is returned.</li>

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

> 122:      * {@return the value associated to the given {@code Class}}
> 123:      * <p>
> 124:      * This method first performs a read-only access, and returns the associated

I'm not 100% sure on the "read-only" label here, as it does recompute-and-store if there is no current value.

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

> 165:     public void remove(Class<?> type) {
> 166:         ClassValueMap map = getMap(type);
> 167:         map.removeAccess(this);

Should this really be "Access" and not "Association"?

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

> 316:     Version<T> version() { return version; }
> 317:     void bumpVersion() { version = new Version<>(this); }
> 318:     static final class Version<T> {

Since this is all final components, it might serve better as a record?

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

> 375:                 return new Entry<>(v2, value);
> 376:             }
> 377:             return this;

The following reads slightly better (to me):

Suggestion:

            return version.refersTo(v2) ? this : new Entry<>(v2, value);

test/jdk/java/lang/invoke/ClassValueTest.java line 179:

> 177: 
> 178:     private static final long COMPUTE_TIME_MILLIS = 100;
> 179:     private static final Duration TIMEOUT = Duration.of(2, ChronoUnit.SECONDS);

In order to reduce spurious failures here, I'd probably put this timeout a bit longer—let's say 5s.

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

PR Review: https://git.openjdk.org/jdk/pull/24043#pullrequestreview-2806354549
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2068235676
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069033640
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069037387
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069047258
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069052518
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069056808