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

[Chen Liang]

On Wed, 30 Apr 2025 09:15:49 GMT, Viktor Klang <vklang at openjdk.org> wrote:

>> 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/153a25f7...221d51e7
>
> 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>

I think I will reword as:
"An association is already created by another attempt. The associated value is returned."

What do you think? An association is either present or absent for a coordinate. In addition, all other list entries are  "What happened. What happens next."

> 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.

Yep, it needs a read-only access to know 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"?

Oh, I named all methods on `ClassValueMap` to `access`. When a remove is done, it might remove a previous remove (no association) in addition to actual associations.

> 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?

Note that this class has a significant identity. Does that make it still suitable for record conversion? I think we need a comment on its identity significance if we convert.

> 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.

Oh, the timeout is not to reduce failures - it mainly aims to prevent accidental infinite loops from stalling tests.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069106581
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069108612
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069109848
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069113085
PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069114518