Repository navigation
streams/xp_socket: eliminate poll() when MSG_DONTWAIT is available - #8092
MaxKellermann wants to merge 2 commits into
Conversation
|
CI failures unrelated to this PR. |
|
Please rebase onto current php-src:master and force-push, to re-trigger CI. |
2f7a776 to
316bbae
Compare
|
Yay, "All checks have passed". |
7758e72 to
a81701a
Compare
|
I rebased; AppVeyor failure unrelated to this PR. |
a81701a to
b151407
Compare
|
It makes sense to me. Do we actually have any test case covering this path? |
|
@MaxKellermann The same optimization can also be applied to
@bukka I believe that many socket-stream-related tests have covered this path, such as the most straightforward case for me: |
|
@MaxKellermann do you still want to proceed with your open PRs ? still few weeks left before the feature freeze :-) |
|
would it require a lot of changes ? otherwise yes it s mergeable as is. |
If there is a zero timeout and MSG_DONTWAIT is available (or the
socket is non-blocking), the poll() call is not necessary, and we can
just call recv() right away.
Before this change:
poll([{fd=4, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 0 (Timeout)
poll([{fd=4, events=POLLIN|POLLERR|POLLHUP}], 1, 60000) = 1 ([{fd=4, revents=POLLIN}])
recvfrom(4, "HTTP/1.1 301 Moved Permanently\r\n"..., 8192, MSG_DONTWAIT, NULL, NULL) = 348
poll([{fd=4, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 1 ([{fd=4, revents=POLLIN}])
recvfrom(4, "", 1, MSG_PEEK, NULL, NULL) = 0
After this change:
recvfrom(4, 0x7ffe0cc719a0, 1, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
poll([{fd=4, events=POLLIN|POLLERR|POLLHUP}], 1, 60000) = 1 ([{fd=4, revents=POLLIN}])
recvfrom(4, "HTTP/1.1 301 Moved Permanently\r\n"..., 8192, MSG_DONTWAIT, NULL, NULL) = 348
recvfrom(4, "", 1, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 0
The first poll() is replaced by recvfrom(), and the third poll() is
omitted completely.
b151407 to
8bdef58
Compare
|
I've added the same optimization to ext/openssl as suggested by @twose, but only if |
|
MACOS_DEBUG_NTS build failure unrelated to this PR. |
|
The Windows build fails, because |
There's a fallback definition in php-src/main/streams/xp_socket.c Lines 30 to 32 in 790be97 Will add it. |
|
Interestingly, |
If there is a zero timeout and MSG_DONTWAIT is available (or the socket is non-blocking), the poll() call is not necessary, and we can just call recv() right away.
8bdef58 to
36ec92c
Compare
|
@MaxKellermann I had a proper look at this. All looks reasonable to me. I think there's a possible optimization in xp_ssl that I just did at #8829 . Please take a look and let me know in case there's a specific reason why you haven't done it in this PR. |
If there is a zero timeout and MSG_DONTWAIT is available (or the
socket is non-blocking), the poll() call is not necessary, and we can
just call recv() right away.
Before this change:
poll([{fd=4, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 0 (Timeout)
poll([{fd=4, events=POLLIN|POLLERR|POLLHUP}], 1, 60000) = 1 ([{fd=4, revents=POLLIN}])
recvfrom(4, "HTTP/1.1 301 Moved Permanently\r\n"..., 8192, MSG_DONTWAIT, NULL, NULL) = 348
poll([{fd=4, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 1 ([{fd=4, revents=POLLIN}])
recvfrom(4, "", 1, MSG_PEEK, NULL, NULL) = 0
After this change:
recvfrom(4, 0x7ffe0cc719a0, 1, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
poll([{fd=4, events=POLLIN|POLLERR|POLLHUP}], 1, 60000) = 1 ([{fd=4, revents=POLLIN}])
recvfrom(4, "HTTP/1.1 301 Moved Permanently\r\n"..., 8192, MSG_DONTWAIT, NULL, NULL) = 348
recvfrom(4, "", 1, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 0
The first poll() is replaced by recvfrom(), and the third poll() is
omitted completely.
[Splitted from https://github.com//pull/8032]