Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253478: (se) epoll Selector should use eventfd for wakeup instead of pipe #2082

Closed
wants to merge 9 commits into from

Conversation

bplb
Copy link
Member

@bplb bplb commented Jan 14, 2021

Please review this change which modifies the Linux epoll(7)-based Selector to use eventfd(2) instead of pipe(2) in its wakeup mechanism. The change passes all tier 1-tier 3 tests on Linux. Based on rudimentary testing, there does not appear to be any appreciable change in performance. One improvement however is that only one file descriptor instead of two is used for the wakeup. No test is included as the code is covered well by existing tests.


Progress

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

Issue

  • JDK-8253478: (se) epoll Selector should use eventfd for wakeup instead of pipe

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2082/head:pull/2082
$ git checkout pull/2082

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 14, 2021

👋 Welcome back bpb! 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 openjdk bot added the rfr label Jan 14, 2021
@openjdk
Copy link

openjdk bot commented Jan 14, 2021

@bplb 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.

@openjdk openjdk bot added the nio label Jan 14, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 14, 2021

@AlanBateman
Copy link
Contributor

AlanBateman commented Jan 14, 2021

@bplb I think it's too premature to ask us to review this until you have some performance data. Do we have any existing micros for wakeup?

@bplb
Copy link
Member Author

bplb commented Jan 14, 2021

Yes. I have one based on the Wakeup test but it is rather ugly.

@bplb
Copy link
Member Author

bplb commented Jan 14, 2021

This simple benchmark below gives results for the epoll(7)-based Selector which are three to four percent faster for the version which uses eventfd(2) for wakeup.

@State(Scope.Thread)
public class EventFDBench {
    private Selector sel;

    @Setup(Level.Iteration)
    public void setup() throws IOException {
        sel = Selector.open();
    }

    @Benchmark
    public int test() throws IOException {
        return sel.wakeup().select();
    }
}

@AlanBateman
Copy link
Contributor

AlanBateman commented Jan 15, 2021

I did experiments with eventfd(2) a few years ago but didn't see any difference at the time. I think it would be useful to include the PR the results from the JMH runs so that there is at least some record of the results.

As regards the patch, I would have expected the only native code is be the method that creates the eventfd instance. The set/reset methods can be implemented with IOUtil.write1/drain1. Also I think the EventFD constructor needs a flag so decide the blocking mode, alternative we ignore it and using the existing IOUtil.configureBlocking.

@mlbridge
Copy link

mlbridge bot commented Jan 15, 2021

Mailing list message from Brian Burkhalter on nio-dev:

On Jan 15, 2021, at 3:31 AM, Alan Bateman <alanb at openjdk.java.net> wrote:

I did experiments with eventfd(2) a few years ago but didn't see any difference at the time. I think it would be useful to include the PR the results from the JMH runs so that there is at least some record of the results.

I?ll do that.

As regards the patch, I would have expected the only native code is be the method that creates the eventfd instance. The set/reset methods can be implemented with IOUtil.write1/drain1.

Reading or writing fewer than 8 bytes from / to the eventfd object will result in an EINVAL error. Yes, IOUtil.drain(int) could be used instead of EventFD.reset(), although its more complex, but there is no write() in IOUtil which does not require a FileDescriptor and a ByteBuffer, which seems like overkill here. A method IOUtil.write(int fd, long value) could be added however.

Also I think the EventFD constructor needs a flag so decide the blocking mode, alternative we ignore it and using the existing IOUtil.configureBlocking.

Or maybe better yet just go with hard-coded blocking or non-blocking and dispense with the parameter.

Brian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210115/250820a9/attachment-0001.htm>

@bplb
Copy link
Member Author

bplb commented Jan 15, 2021

Benchmark output for commit 3:

1 fork, 5 10s warmup iterations, 10 10s measurement iterations, 1 thread
pipe wakeup

Result "org.sample.EventFDBench.test":
  674120.350 ±(99.9%) 2246.122 ops/s [Average]
  (min, avg, max) = (671278.574, 674120.350, 676866.600), stdev = 1485.671
  CI (99.9%): [671874.229, 676366.472] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  676831.218 ±(99.9%) 2353.488 ops/s [Average]
  (min, avg, max) = (673745.955, 676831.218, 678758.514), stdev = 1556.686
  CI (99.9%): [674477.731, 679184.706] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  675830.928 ±(99.9%) 3052.319 ops/s [Average]
  (min, avg, max) = (671286.540, 675830.928, 679293.739), stdev = 2018.920
  CI (99.9%): [672778.609, 678883.246] (assumes normal distribution)

eventfd wakeup

Result "org.sample.EventFDBench.test":
  698126.388 ±(99.9%) 2776.885 ops/s [Average]
  (min, avg, max) = (694253.565, 698126.388, 700086.784), stdev = 1836.738
  CI (99.9%): [695349.503, 700903.273] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  699992.095 ±(99.9%) 3653.650 ops/s [Average]
  (min, avg, max) = (695504.040, 699992.095, 702932.998), stdev = 2416.663
  CI (99.9%): [696338.445, 703645.746] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  702140.433 ±(99.9%) 3019.100 ops/s [Average]
  (min, avg, max) = (698291.810, 702140.433, 704818.744), stdev = 1996.948
  CI (99.9%): [699121.333, 705159.533] (assumes normal distribution)


1 fork, 5 10s warmup iterations, 10 10s measurement iterations, 6 threads
pipe wakeup

Result "org.sample.EventFDBench.test":
  3223142.871 ±(99.9%) 94755.560 ops/s [Average]
  (min, avg, max) = (3120710.890, 3223142.871, 3283407.281), stdev = 62674.936
  CI (99.9%): [3128387.312, 3317898.431] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  3260747.260 ±(99.9%) 11514.840 ops/s [Average]
  (min, avg, max) = (3248875.268, 3260747.260, 3268729.421), stdev = 7616.354
  CI (99.9%): [3249232.419, 3272262.100] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  3243306.245 ±(99.9%) 56893.385 ops/s [Average]
  (min, avg, max) = (3160345.608, 3243306.245, 3277487.093), stdev = 37631.452
  CI (99.9%): [3186412.859, 3300199.630] (assumes normal distribution)

eventfd wakeup

Result "org.sample.EventFDBench.test":
  3340323.956 ±(99.9%) 38169.834 ops/s [Average]
  (min, avg, max) = (3280464.427, 3340323.956, 3355415.702), stdev = 25246.982
  CI (99.9%): [3302154.122, 3378493.790] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  3344287.954 ±(99.9%) 46249.831 ops/s [Average]
  (min, avg, max) = (3261670.180, 3344287.954, 3363007.332), stdev = 30591.400
  CI (99.9%): [3298038.123, 3390537.786] (assumes normal distribution)

Result "org.sample.EventFDBench.test":
  3353728.294 ±(99.9%) 40760.876 ops/s [Average]
  (min, avg, max) = (3285776.202, 3353728.294, 3372002.914), stdev = 26960.796
  CI (99.9%): [3312967.418, 3394489.170] (assumes normal distribution)

@bplb
Copy link
Member Author

bplb commented Jan 15, 2021

Throughput improvement as measured by the benchmark looks to be a bit over 3% for eventfd wakeup with respect to pipe wakeup. The confidence intervals of the pipe wakeup results mostly do not overlap those of the corresponding eventfd wakeup results which tends to suggest some degree of reliability.

Summary of benchmark results:

1 fork, 5 10s warmup iterations, 10 10s measurement iterations, 1 thread
pipe wakeup

Mean average: 675594.1653333333
Max CI upper bound: 679184.706

eventfd wakeup

Mean average: 700086.3053333334
Min CI lower bound: 695349.503
Throughput change: +3.6%

1 fork, 5 10s warmup iterations, 10 10s measurement iterations, 6 threads
pipe wakeup

Mean average: 3242398.7919999994
Max CI upper bound: 3317898.431

eventfd wakeup

Mean average: 3346113.4013333335
Min CI lower bound: 3298038.123
Throughput change: +3.2%

@AlanBateman
Copy link
Contributor

AlanBateman commented Jan 18, 2021

Are you going to include the micro benchmark in the patch? The presentation of the summary in the comments is hard to read but a 3% is okay. It was inconclusive when I tried a long time ago.

The updated patch looks better. IOUtil.write(fd, long) begs the question as to whether the 8 bytes for the long are written in big or little endian. I realise it doesn't matter for EventFD but we will need to get this right. I would be tempted to call evetnfd with the flags set to 0 and then use IOUtil.configureBlocking when we need it non-blocking.

… reinstate EventFD.set0(), use IOUtil.configureBlocking().
@openjdk openjdk bot removed the rfr label Jan 19, 2021
@openjdk openjdk bot added the rfr label Jan 19, 2021
@bplb
Copy link
Member Author

bplb commented Jan 19, 2021

Updated the patch to:

  • include the micro benchmark,
  • create the eventfd with zero initial value and flags,
  • use IOUtil.configureBlocking() to set the eventfd to non-blocking,
  • remove the addition of IOUtil.write(int fd, long value) and its use in EventFD.set(),
  • add EventFD.set0() and use it in EventFD.set().

long fds = IOUtil.makePipe(false);
this.fd0 = (int) (fds >>> 32);
this.fd1 = (int) fds;
this.eventfd = new EventFD();
} catch (IOException ioe) {
EPoll.freePollArray(pollArrayAddress);
FileDispatcherImpl.closeIntFD(epfd);
throw ioe;
}

// register one end of the socket pair for wakeups
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the comment at line 89 no longer applies.

Copy link
Member Author

@bplb bplb Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, thanks.

FileDispatcherImpl.closeIntFD(efd);
}

static native int eventfd0() throws IOException;
Copy link
Contributor

@AlanBateman AlanBateman Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the native methods should be private.

Copy link
Member Author

@bplb bplb Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So changed.

*/
EventFD() throws IOException {
efd = eventfd0();
IOUtil.configureBlocking(IOUtil.newFD(efd), false);
Copy link
Contributor

@AlanBateman AlanBateman Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is just one no-arg constructor then I think it should create the eventfd with the default settings, meaning blocking mode. A second constructor that takes a boolean blocking parameter would be okay too.

Copy link
Member Author

@bplb bplb Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above suggestions addressed.

* @param the integral eventfd file descriptor
* @return the number of bytes written; should equal 8
*/
static private native int set0(int efd) throws IOException;
Copy link
Contributor

@AlanBateman AlanBateman Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit but we usually put the private modifier first.

*/
EventFD(boolean blocking) throws IOException {
efd = eventfd0();
IOUtil.configureBlocking(IOUtil.newFD(efd), blocking);
Copy link
Contributor

@AlanBateman AlanBateman Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay but the addition of the blocking parameter means you can go back to one of the early iterations and just calling eventfd with the EFD_NONBLOCK. Up to you.

Copy link
Member Author

@bplb bplb Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand this comment as a previous comment preferred IOUtil.configureBlocking() to set the blocking state.

Copy link
Contributor

@AlanBateman AlanBateman Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous comments were in the context of the no-arg EventFD constructor where we expected it would be created in blocking mode. In that context EPollSelectorImpl would use IOUtil.configureBlocking to configure it to non-blocking. In the new version, there is a blocking parameter so EventFD can use IOUtil.configureBlocking when blocking is "false", or specify EFD_NONBLOCK to eventfd, either is okay with me.

@openjdk
Copy link

openjdk bot commented Jan 21, 2021

@bplb 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:

8253478: (se) epoll Selector should use eventfd for wakeup instead of pipe

Reviewed-by: alanb

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 145 new commits pushed to the master branch:

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.

@openjdk openjdk bot added the ready label Jan 21, 2021
@bplb
Copy link
Member Author

bplb commented Jan 21, 2021

/integrate

@openjdk openjdk bot closed this Jan 21, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 21, 2021
@openjdk
Copy link

openjdk bot commented Jan 21, 2021

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

Your commit was automatically rebased without conflicts.

Pushed as commit a8073ef.

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

@bplb bplb deleted the 8253478-eventfd branch Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated nio
3 participants