RFR: 8338858: Replace undocumented APIs with documented APIs in the OpenJDK

Daniel Jeliński djelinski at openjdk.org
Mon Sep 9 16:39:05 UTC 2024


On Thu, 22 Aug 2024 18:18:57 GMT, Dhamoder Nalla <dhanalla at openjdk.org> wrote:

> The `wepoll` code has been ported from opensource repo [`wepoll`](https://github.com/piscisaureus/wepoll) to OpenJDK in PR [pull/3816](https://github.com/openjdk/jdk/pull/3816), which incorporated undocumented Windows APIs (NtCreateKeyedEvent, NtReleaseKeyedEvent, NtWaitForKeyedEvent). 
> 
> This PR aims to replace these undocumented APIs with documented synchronization APIs. 
> 
> Test Performed:
> 1. All 12 tests in wepoll repo passed 
>            test-auto-drop-on-close PASS
>            test-connect-fail-events PASS
>            test-connect-success-events PASS
>            test-ctl-fuzz PASS
>            test-error PASS
>            test-leak-1 PASS
>            test-mixed-socket-types PASS
>            test-multi-poll PASS
>            test-oneshot-and-hangup PASS
>            test-reflock PASS
>            test-tree PASS
>            test-udp-pings PASS
>         DONE - 12 PASSED, 0 FAILED
> 
> 2. No new failure in JTreg test
> 3. Micro bench results: similar performance score before and after the changes
> 
> without change in this PR:
> 
> ![image](https://github.com/user-attachments/assets/c784d00e-3f0a-483d-b60a-c7a46aba885c)
> 
> With the change in this PR:
> 
> ![image](https://github.com/user-attachments/assets/5e6a68bc-48ae-475f-891e-395941068f6e)

I understand that this is a like-for-like replacement, where `reflock__signal_event` waits for another thread to call `reflock__await_event`, because that's what `NtReleaseKeyedEvent` did, even if that is not necessary from the perspective of the calling code. It looks OK, but it doesn't make the code easier to follow.

I think if we're going to touch this code, we should make it more readable than the original. We could replace the whole reflock structure with SRW locks; `reflock_ref` is semantically equivalent to `AcquireSRWLockShared`, `reflock_unref` is equivalent to `ReleaseSRWLockShared`, and `reflock_unref_and_destroy` is equivalent to `ReleaseSRWLockShared` followed by `AcquireSRWLockExclusive`.

Also, I'd very much like to avoid forking wepoll here. We keep our own copy to simplify the build system, but we don't want to maintain our own fork. @dhanalla if the https://github.com/piscisaureus/wepoll/pull/37 request is not accepted, do you plan to maintain a fork of wepoll? Or is wepoll officially dead?

Finally, this PR claims to replace the undocumented APIs, but does nothing about the usage of AFD. Is there a plan to remove the AFD too?

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

PR Review: https://git.openjdk.org/jdk/pull/20682#pullrequestreview-2290417466


More information about the nio-dev mailing list