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:
>
> 
>
> With the change in this PR:
>
> 
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