On Wed, 30 Apr 2025 23:02:24 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Use a thread-safe ReferencedKeySet instead of a WeakHashMap key set. > > 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 three additional commits since > the last revision: > > - Add a test case, fails isWrapperInstance on mainline > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/mhp-wrapper-check-async > - 8350549: MethodHandleProxies.WRAPPER_TYPES is not thread-safe Thank you Chen for adding the test. The test looks OK to me. Please add `8350549` to the `@bug` in the test definition. I think a brief comment on the test method would make it easier to understand what it does. Would you consider adding a comment, something like: /** * The test verifies that MethodHandleProxies.asInterfaceInstance() and * MethodHandleProxies.isWrapperInstance() work as expected when invoked concurrently. */ Finally, given the nature of this new test, I think it would be good to have it run in our CI with a test repeat of around 50 to be sure that it doesn't fail intermittently. test/jdk/java/lang/invoke/MethodHandleProxies/BasicTest.java line 275: > 273: .limit(100) > 274: .forEach(inst -> > assertTrue(MethodHandleProxies.isWrapperInstance(inst), > 275: () -> Objects.toIdentityString(inst))); Nit - the failure message (if at all it fails for any reason) could perhaps be: Objects.toIdentityString(inst) + " was expected to be a wrapper instance, but isn't" ------------- PR Review: https://git.openjdk.org/jdk/pull/23757#pullrequestreview-2810048465 PR Review Comment: https://git.openjdk.org/jdk/pull/23757#discussion_r2070418275
