8266369: (se) Add wepoll based Selector #3816

AlanBateman · 2021-04-30T14:04:46Z

This PR proposes a new Selector implementation for the Windows platform based on the Bert Belder's "wepoll" library. The wepoll library provides a scalable polling interface to the Ancillary Function Driver for Winsock. The interface that it exposes is close to the Linux epoll interface. The main benefit is that the cost of selection operations are a function of the number of sockets that are ready for I/O rather than the number of channels registered with the Selector. The existing Selector on Windows uses the legacy "select" which scales horribly, partly because it's O(N) on the number of registered channels and partly because it is limited to 1024 sockets and so requires a thread per 1024 sockets.

The patch brings the wepoll sources into the repo, adds a small shim, and the Selector implementation itself. The new implementation needs to handle OOB data in the same way as the old implementation so the method to discard OOB moves to Net.

I've added a microbenchmark to demonstrate the scaling when there is 1, 10, ... 10k connections and one socket ready for I/O. Windows Server 2019 was used for the data here. I've seen improvements with almost everything that I've tried. The only exception is a select after a wakeup where the old implementation skips the selection operation (arguably a long standing bug in the old implementation).

Existing select based Selector where the selection operation is almost 1ms when there are 10,000 channels registered.

Benchmark                (nchannels)  Mode  Cnt       Score       Error  Units
SelectOne.testSelectOne            1  avgt   20    1587.429 ±     4.053  ns/op
SelectOne.testSelectOne           10  avgt   20    2515.324 ±     8.141  ns/op
SelectOne.testSelectOne          100  avgt   20   10903.445 ±    65.508  ns/op
SelectOne.testSelectOne         1000  avgt   20  101651.369 ±   673.781  ns/op
SelectOne.testSelectOne        10000  avgt   20  932032.385 ± 59004.246  ns/op

New wepoll based Selector:

Benchmark                (nchannels)  Mode  Cnt     Score    Error  Units
SelectOne.testSelectOne            1  avgt   20  1428.914 ±  6.648  ns/op
SelectOne.testSelectOne           10  avgt   20  1429.806 ±  5.537  ns/op
SelectOne.testSelectOne          100  avgt   20  1433.371 ±  3.253  ns/op
SelectOne.testSelectOne         1000  avgt   20  1435.007 ± 12.420  ns/op
SelectOne.testSelectOne        10000  avgt   20  1445.201 ±  7.902  ns/op

Replacing the Selector implementation after 19 years of service is a big step. The proposal is to leave the existing implementation until there is confidence with the new implementation. It also serves as a backup in case Windows were to significantly change the AFD interface. The old Selector can be used by running with -Djava.nio.channels.spi.SelectorProvider=sun.nio.ch.WindowsSelectorProvider. A selection of existing tests have been updated to run a second time with the old Selector so that it is continues to be tested.

CSR and Release Note planned.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8266369: (se) Add wepoll based Selector

Reviewers

Chris Hegarty (@ChrisHegarty - Reviewer)
Michael McMahon (@Michael-Mc-Mahon - Reviewer) ⚠️ Review applies to 221af5a
Vyom Tewari (@vyommani - Committer) ⚠️ Review applies to 221af5a
Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to fda20ec
Brian Burkhalter (@bplb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3816/head:pull/3816
$ git checkout pull/3816

Update a local copy of the PR:
$ git checkout pull/3816
$ git pull https://git.openjdk.java.net/jdk pull/3816/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3816

View PR using the GUI difftool:
$ git pr show -t 3816

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3816.diff

bridgekeeper · 2021-04-30T14:05:24Z

👋 Welcome back alanb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-04-30T14:06:58Z

@AlanBateman The following label will be automatically applied to this pull request:

nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

AlanBateman · 2021-04-30T14:27:05Z

/issue JDK-8266369

openjdk · 2021-04-30T14:27:47Z

@AlanBateman This issue is referenced in the PR title - it will now be updated.

mlbridge · 2021-04-30T17:25:28Z

Webrevs

src/java.base/windows/classes/sun/nio/ch/WEPollSelectorImpl.java

src/java.base/windows/native/libnio/ch/WEPollNatives.c

vyommani · 2021-05-04T06:02:34Z

src/java.base/windows/classes/sun/nio/ch/WEPoll.java

+     * Returns event[i];
+     */
+    static long getEvent(long address, int i) {
+        return address + (SIZEOF_EPOLLEVENT*i);


space issue (SIZEOF_EPOLLEVENT*i);

src/java.base/windows/native/libnio/ch/IOUtil.c

src/java.base/windows/classes/sun/nio/ch/WEPoll.java

test/micro/org/openjdk/bench/java/nio/channels/SelectOne.java

test/jdk/java/nio/channels/Selector/RacyDeregister.java

src/java.base/share/classes/sun/nio/ch/Net.java

src/java.base/windows/native/libnio/ch/Net.c

src/java.base/windows/native/libnio/ch/WEPollNatives.c

ChrisHegarty

Overall, this looks like a very nice addition.

src/java.base/windows/classes/sun/nio/ch/WEPoll.java

openjdk · 2021-05-05T15:27:19Z

@AlanBateman This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8266369: (se) Add wepoll based Selector

Reviewed-by: chegar, michaelm, vtewari, dfuchs, bpb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 16 new commits pushed to the master branch:

0790e60: 8196743: jstatd doesn't see new Java processes inside Docker container
c6aa8f1: 8232644: bugs in serialized-form.html
b5b3119: 8266589: (fs) Improve performance of Files.copy() on macOS using copyfile(3)
947d69d: 8265042: javadoc HTML files not generated for types nested in records
946b0fe: 8266645: javac should not check for sealed supertypes in intersection types
74fecc0: 8266503: [UL] Make Decorations safely copy-able and reduce their size
86b8dc9: 8265426: Update java.security to use instanceof pattern variable
3fcdc50: 8266646: Add more diagnostic to java/lang/System/LoggerFinder/modules
9a19a0c: 8264760: JVM crashes when two threads encounter the same resolution error
14f0afe: 8214237: Join parallel phases post evacuation
... and 6 more: https://git.openjdk.java.net/jdk/compare/ebb68d2b8652328b80780f6a39c78ff19f24136a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

vyommani

LGTM

test/micro/org/openjdk/bench/java/nio/channels/SelectOne.java

bplb

A good improvement. Approved.

AlanBateman · 2021-05-08T07:13:17Z

/integrate

openjdk · 2021-05-08T07:14:58Z

@AlanBateman Since your change was applied there have been 18 commits pushed to the master branch:

ff77ca8: 8266675: Optimize IntHashTable for encapsulation and ease of use
04fad70: 8266765: [BACKOUT] JDK-8255493 Support for pre-generated java.lang.invoke classes in CDS dynamic archive
0790e60: 8196743: jstatd doesn't see new Java processes inside Docker container
c6aa8f1: 8232644: bugs in serialized-form.html
b5b3119: 8266589: (fs) Improve performance of Files.copy() on macOS using copyfile(3)
947d69d: 8265042: javadoc HTML files not generated for types nested in records
946b0fe: 8266645: javac should not check for sealed supertypes in intersection types
74fecc0: 8266503: [UL] Make Decorations safely copy-able and reduce their size
86b8dc9: 8265426: Update java.security to use instanceof pattern variable
3fcdc50: 8266646: Add more diagnostic to java/lang/System/LoggerFinder/modules
... and 8 more: https://git.openjdk.java.net/jdk/compare/ebb68d2b8652328b80780f6a39c78ff19f24136a...master

Your commit was automatically rebased without conflicts.

Pushed as commit be4f25b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Initial commit

8238367

openjdk bot added the nio nio-dev@openjdk.org label Apr 30, 2021

openjdk bot changed the title ~~8266369: Add wepoll based Selector~~ 8266369: (se) Add wepoll based Selector Apr 30, 2021

AlanBateman marked this pull request as ready for review April 30, 2021 17:20

openjdk bot added the rfr Pull request is ready for review label Apr 30, 2021

Move SelectorWakeup micro to channels directory

fbf98d8

vyommani reviewed May 4, 2021

View reviewed changes

src/java.base/windows/classes/sun/nio/ch/WEPollSelectorImpl.java Outdated Show resolved Hide resolved

vyommani reviewed May 4, 2021

View reviewed changes

src/java.base/windows/classes/sun/nio/ch/WEPollSelectorImpl.java Show resolved Hide resolved

AlanBateman added 2 commits May 4, 2021 06:20

Use 'to' instead of '(int) timeout

abd23dc

Merge

3431181

vyommani reviewed May 4, 2021

View reviewed changes

src/java.base/windows/native/libnio/ch/WEPollNatives.c Outdated Show resolved Hide resolved

vyommani reviewed May 4, 2021

View reviewed changes

AlanBateman added 2 commits May 5, 2021 06:58

Propagate exception from discardOOB, cleanup

8906d25

Merge

50ea54c

Michael-Mc-Mahon reviewed May 5, 2021

View reviewed changes

src/java.base/windows/native/libnio/ch/IOUtil.c Show resolved Hide resolved

Michael-Mc-Mahon reviewed May 5, 2021

View reviewed changes

src/java.base/windows/classes/sun/nio/ch/WEPoll.java Show resolved Hide resolved

ChrisHegarty reviewed May 5, 2021

View reviewed changes

test/micro/org/openjdk/bench/java/nio/channels/SelectOne.java Outdated Show resolved Hide resolved

test/jdk/java/nio/channels/Selector/RacyDeregister.java Show resolved Hide resolved

src/java.base/share/classes/sun/nio/ch/Net.java Show resolved Hide resolved

AlanBateman added 2 commits May 5, 2021 10:43

Fix copyright header on SelectOne benchmark

8c9551a

Remove unused WEPoll.getDescriptor. Update micro to close listener

8103c0e

dfuch reviewed May 5, 2021

View reviewed changes

src/java.base/windows/native/libnio/ch/Net.c Show resolved Hide resolved

epoll_create1 returns NULL on error

221af5a

pconcannon reviewed May 5, 2021

View reviewed changes

src/java.base/windows/native/libnio/ch/WEPollNatives.c Outdated Show resolved Hide resolved

ChrisHegarty approved these changes May 5, 2021

View reviewed changes

src/java.base/windows/classes/sun/nio/ch/WEPoll.java Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label May 5, 2021

Michael-Mc-Mahon approved these changes May 6, 2021

View reviewed changes

vyommani approved these changes May 6, 2021

View reviewed changes

AlanBateman added 2 commits May 6, 2021 13:28

Bring back getDescriptor, bzero poll array when initially created

b4f09e2

Merge

260b13f

dfuch reviewed May 6, 2021

View reviewed changes

test/micro/org/openjdk/bench/java/nio/channels/SelectOne.java Outdated Show resolved Hide resolved

Update micro to bind to loopback address

fda20ec

dfuch approved these changes May 6, 2021

View reviewed changes

AlanBateman added 2 commits May 7, 2021 07:41

Windows 32-bit support

5380d87

Merge

d2be1c3

ChrisHegarty approved these changes May 7, 2021

View reviewed changes

bplb approved these changes May 7, 2021

View reviewed changes

openjdk bot closed this May 8, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 8, 2021

8266369: (se) Add wepoll based Selector #3816

8266369: (se) Add wepoll based Selector #3816

AlanBateman commented Apr 30, 2021 •

edited by openjdk bot

bridgekeeper bot commented Apr 30, 2021

openjdk bot commented Apr 30, 2021

AlanBateman commented Apr 30, 2021

openjdk bot commented Apr 30, 2021

mlbridge bot commented Apr 30, 2021 •

edited

vyommani May 4, 2021

ChrisHegarty left a comment

openjdk bot commented May 5, 2021 •

edited

vyommani left a comment

bplb left a comment

AlanBateman commented May 8, 2021

openjdk bot commented May 8, 2021

8266369: (se) Add wepoll based Selector #3816

8266369: (se) Add wepoll based Selector #3816

Conversation

AlanBateman commented Apr 30, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 30, 2021

openjdk bot commented Apr 30, 2021

AlanBateman commented Apr 30, 2021

openjdk bot commented Apr 30, 2021

mlbridge bot commented Apr 30, 2021 • edited

Webrevs

vyommani May 4, 2021

Choose a reason for hiding this comment

ChrisHegarty left a comment

Choose a reason for hiding this comment

openjdk bot commented May 5, 2021 • edited

vyommani left a comment

Choose a reason for hiding this comment

bplb left a comment

Choose a reason for hiding this comment

AlanBateman commented May 8, 2021

openjdk bot commented May 8, 2021

AlanBateman commented Apr 30, 2021 •

edited by openjdk bot

mlbridge bot commented Apr 30, 2021 •

edited

openjdk bot commented May 5, 2021 •

edited