RFR: 8338858: Replace undocumented APIs with documented APIs in the OpenJDK
Dhamoder Nalla
dhanalla at openjdk.org
Fri Sep 20 22:14:40 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 [piscisaureus/wepoll#37](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?
Thank you @djelinski, At this time, we don’t have the bandwidth to fork and maintain wepoll. If this presents a challenge for the PR, we completely understand and will proceed by closing it. We will make the necessary changes directly in our downstream distribution.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20682#issuecomment-2364687200
More information about the nio-dev
mailing list