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

wepoll: Use wepoll to light up the epoll backend on Windows #1006

Merged
merged 1 commit into from
May 8, 2020
Merged

wepoll: Use wepoll to light up the epoll backend on Windows #1006

merged 1 commit into from
May 8, 2020

Conversation

nigriMSFT
Copy link
Contributor

libevent is lacking a scalable backend on Windows. Let's leverage the wepoll
library until Windows comes up with an epoll/kqueue compete user mode API.

-All regress tests pass for standard wepoll
-These 2 tests fail intermittently for changelist wepoll, so disabling
changelist wepoll for now
http/cancel_inactive_server
http/stream_in
-verify target on Windows runs tests for both wepoll and win32 backends
-wepoll backend preferred over win32 backend
-wepoll version 1.5.6

@azat
Copy link
Member

azat commented Apr 28, 2020

Looks very promising! (but I need to read what is wepoll at least briefly)

@nigriMSFT
Copy link
Contributor Author

The existing epoll backend logic was used to illustrate how much code can be shared between epoll and wepoll backends. wepoll backend can certainly be pulled out into its own module if preferred.

@azat
Copy link
Member

azat commented Apr 28, 2020

The existing epoll backend logic was used to illustrate how much code can be shared between epoll and wepoll backends. wepoll backend can certainly be pulled out into its own module if preferred.

The diff is pretty small, so I think that the way you go is preferable

@azat
Copy link
Member

azat commented Apr 28, 2020

Cc: @ygj6

@nigriMSFT
Copy link
Contributor Author

The default backend on Windows can remain as win32 (select) if we want to give the new backend time to bake with opt-in consumers. Changing the default was more of a convenience thing as I was piecing this together.

@azat
Copy link
Member

azat commented Apr 28, 2020

The default backend on Windows can remain as win32 (select) if we want to give the new backend time to bake with opt-in consumers. Changing the default was more of a convenience thing as I was piecing this together.

I guess that for better adoption default can be changed, in the upstream, anyway most of users will use some ancient version that has select by default

CMakeLists.txt Outdated Show resolved Hide resolved
@nigriMSFT
Copy link
Contributor Author

Some benchmark data gathered with the following command
./bench -n <total connections> -a 1 -w 1000 -m <backend method>

image

The median of the 25 data points collected by bench is what is reported in the graph. The machines used were ubuntu server 18.04 and Windows Server insider build VMs running on top of Hyper-V. Each VM was allocated 8 VPs and 8192GB memory.

Stopped at 10000 total connections out of convenience. On Windows, evutil_socketpair with AF_UNIX fails after 65536 socket pairs due to a limitation with GetTempFileName (which its use in libevent is already a workaround for lack of abstract sockets support). We can work around this limitation, but even then creating many AF_UNIX sockets on Windows is painfully slow.

Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

Looks good! Except small note around dist archive.

FWIW we have some code that uses io ports directly (event_iocp.c), I'm wondering do we need this now, or we can simply suggest wepoll and that's it?

CMakeLists.txt Show resolved Hide resolved
event.c Outdated Show resolved Hide resolved
@nigriMSFT
Copy link
Contributor Author

nigriMSFT commented May 7, 2020

Hm, but if we do not enable it for cygwin/mingw for what it will be enabled?

It is enabled for native Windows build. The documentation and CI indicate that this project can be built using Visual Studio Build Tools, so that's what I used to develop and test with. The wepoll backend is included if the project is built using those tools.

I was wondering if the released archived dists are only buildable on platforms that support autotools (which would mean buildable on mingw, but not VSBT). I didn't get a clear answer from the other thread. If the above is true, then we certainly need to ensure that wepoll backend is included when building from mingw.

@azat
Copy link
Member

azat commented May 8, 2020

The documentation and CI indicate that this project can be built using Visual Studio Build Tools

But you cannot use autotools for this, you are using cmake in this case.
The autotools can be used for cygwin/mingw build which does not support wepoll.

I was wondering if the released archived dists are only buildable on platforms that support autotools (which would mean buildable on mingw, but not VSBT).

Put it simple, dist archive = git archive HEAD (IOW everything you need to build libevent from sources)

I can fix this by myself and do a final rebase (with push force into this PR), if you don't mind?

@nigriMSFT
Copy link
Contributor Author

The autotools can be used for cygwin/mingw build which does not support wepoll

31a71e4, ff1178f and e2edb7c adds wepoll to the mingw build with both autotools and cmake. If there is anything else that is needed to make this PR ready then feel free to push to it

libevent is lacking a scalable backend on Windows. Let's leverage the wepoll
library until Windows comes up with an epoll/kqueue compete user mode API.

- All regress tests pass for standard wepoll
- These 2 tests fail intermittently for changelist wepoll, so disabling
  changelist wepoll for now
     http/cancel_inactive_server
     http/stream_in
- verify target on Windows runs tests for both wepoll and win32 backends
- wepoll backend preferred over win32 backend
- wepoll version 1.5.6

v2: cleaner backend abstraction. Disallow wepoll on MinGW/Cygwin.
v3: Add wepoll.h to dist
v4: Make sure wepoll source files are excluded from cygwin/mingw builds
v5: Keep win32 as default backend on windows.
v6: Include wepoll in mingw builds. Verified that regress tests pass w/ WEPOLL backend.
v7: Enable wepoll on mingw when building with cmake
v8: Add wepoll testrunner for autotools test target
@azat azat merged commit 83ef321 into libevent:master May 8, 2020
@azat
Copy link
Member

azat commented May 8, 2020

Squashed & merged, many thanks!

@azat
Copy link
Member

azat commented May 8, 2020

31a71e4, ff1178f and e2edb7c adds wepoll to the mingw build with both autotools and cmake

Nice catch!

@nigriMSFT nigriMSFT deleted the wepoll-backend branch May 8, 2020 21:53
@ygj6
Copy link
Member

ygj6 commented May 14, 2020

Sorry I'm late.
I tested the win32 and wepoll backends using the bench that comes with libevent.
My test environment:

Windows10 ( CPU: i7-8700  RAM:32G  SystemType:64bit )
Visual Studio 2019

Command:

bench -n <connections> -a 1 -w 1000 -m win32/wepoll

Test results:
image
Conclusion:
After the number of connections is greater than 100, wepoll takes significantly less time than win32, which means that wepoll performs better than win32.

@nigriMSFT
Copy link
Contributor Author

wepoll performs better than win32.

Using the same benchmark on my setup, I came to the same conclusion.
If it is a possibility, what would it take to change the default backend to wepoll? More in-depth performance analysis? Opt-in wepoll use in a production application?

@ygj6
Copy link
Member

ygj6 commented May 14, 2020

I think we need to observe for a while to see the feedback from the developers. win32 has been used for a long time, although it seems a bit less efficient, it is very stable.

@ygj6
Copy link
Member

ygj6 commented May 14, 2020

In addition, for production applications, we can use popular tools to test, such as apache bench. The conclusion will be recognized by more people

@azat
Copy link
Member

azat commented May 14, 2020

I think we need to observe for a while to see the feedback from the developers

I guess we cannot consider this as an option, since I think that very few people will try it, and even fewer (or 0) will came back with feedback.

I was thinking about more tests using some real use cases (http, bufferevent proxying - see becat, and so on)

And once this will be done, we will make it default engine, and I don't that it should take too much time, I would say that not more then month (provided that someone will do these benchmarks)

@ygj6
Copy link
Member

ygj6 commented May 20, 2020

I tested sample/http-server using apache bench.

  • POST 1K file

Command:
ab -n <requests> -c <concurrency> -T "text/plain" -p "D:\testfile_1k.txt" http://127.0.0.1:8000/

Result:
image
image

  • POST 10K file

Command:
ab -n <requests> -c <concurrency> -T "text/plain" -p "D:\testfile_10k.txt" http://127.0.0.1:8000/

Result:
image
image

From the two indicators of throughput and response time, there is no significant change between win32 and wepoll.(I also tested the GET method, and even tested my own production application, and all got similar results.) But why in the sample/bench test mentioned above, the performance of wepoll obviously exceeds that of win32? (Especially when the number of connections is large). I compared the two codes of bench.c and http-server.c, and found that a large number of events were added by event_add() in bench.c, but in http-server.c, event_add() was called very few times. This may indicate that wepoll can only play its superiority when there are many events.

@piscisaureus
Copy link

Hello, wepoll author here... Of course I find it interesting to see libevent considering a switch to wepoll.

W.r.t. the ab benchmarks posted above: were they run from a windows machine (perhaps from the same machine even?).
Apache bench uses select() on windows and therefore I believe that becomes the bottleneck when testing with higher concurrency numbers.

@ygj6
Copy link
Member

ygj6 commented May 20, 2020

when testing with higher concurrency numbers.

Ok,I will try it

@ygj6
Copy link
Member

ygj6 commented May 20, 2020

And What do you think is the better concurrency numbers?

@piscisaureus
Copy link

piscisaureus commented May 20, 2020

@ygj6 I think we have a misunderstanding. There are no better/worse concurrency numbers and I'm not suggesting that you should test with a particular one.

What I'm saying is that if you use 'ab' (running on windows) to benchmark libevent, 'ab' will become the bottleneck and you're not getting any wiser w.r.t. libevent performance.

My suggestion would be to use a different benchmarking tool (one that uses IOCP) or use 'ab' running on a linux system.

azat pushed a commit to azat/libevent that referenced this pull request Jun 28, 2020
Replace close with evutil_closesocket
Caught with PR libevent#1006

(cherry picked from commit 06a1192)
@azat azat mentioned this pull request Apr 3, 2022
21 tasks
@phit666
Copy link

phit666 commented Jul 24, 2022

Looks good! Except small note around dist archive.

FWIW we have some code that uses io ports directly (event_iocp.c), I'm wondering do we need this now, or we can simply suggest wepoll and that's it?

Please keep it as the existing IOCP implementation is good and stable enough (atleast with TCP), your current implementation of wepoll is'nt using acceptex and connectex, APIs that really took advantage of IOCP. The other thing is with the current wepoll implementation, epoll dispatcher is not dispatching any event when server is closing a socket (::closesocket), normally GetQueuedCompletionStatus/eX will return a receive IO with 0 bytes size and this is a good indication that a socket has been closed, the current IOCP implementation is dispatching this event.

@azat
Copy link
Member

azat commented Jul 26, 2022

@phit666 good notes, thank you! How about submitting a patch with this notes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants