Re: rpcrt4: Use standard Wine list to store connections in RpcServerProtseq.
Jacek Caban <jacek(a)codeweavers.com> writes:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/rpcrt4/rpc_binding.h | 2 +- dlls/rpcrt4/rpc_server.c | 18 ++++---- dlls/rpcrt4/rpc_server.h | 2 +- dlls/rpcrt4/rpc_transport.c | 110 +++++++++++++++----------------------------- 4 files changed, 50 insertions(+), 82 deletions(-)
This breaks the Windows build: i686-w64-mingw32-gcc -c -o rpc_transport.o ../../../wine/dlls/rpcrt4/rpc_transport.c -I. -I../../../wine/dlls/rpcrt4 \ -I../../include -I../../../wine/include -D__WINESRC__ -D_RPCRT4_ -DMSWMSG -D_REENTRANT -Wall -pipe \ -fno-strict-aliasing -Wdeclaration-after-statement -Wempty-body -Wignored-qualifiers \ -Wshift-overflow=2 -Wstrict-prototypes -Wtype-limits -Wunused-but-set-parameter -Wvla \ -Wwrite-strings -Wpointer-arith -Wlogical-op -gdwarf-2 -gstrict-dwarf -fno-omit-frame-pointer \ -g -O2 -fno-diagnostics-show-caret -D_WIN32 In file included from ../../../wine/include/windef.h:266:0, from ../../../wine/include/windows.h:37, from ../../../wine/include/winsock.h:110, from ../../../wine/include/winsock2.h:47, from ../../../wine/include/ws2tcpip.h:22, from ../../../wine/dlls/rpcrt4/rpc_transport.c:40: ../../../wine/dlls/rpcrt4/rpc_transport.c: In function ‘rpcrt4_protseq_sock_get_wait_array’: ../../../wine/dlls/rpcrt4/rpc_transport.c:1767:37: error: ‘RpcServerProtseq {aka struct _RpcServerProtseq}’ has no member named ‘conn’ ../../../wine/include/winnt.h:760:21: note: in definition of macro ‘CONTAINING_RECORD’ ../../../wine/dlls/rpcrt4/rpc_transport.c:1772:46: error: ‘RpcConnection {aka struct _RpcConnection}’ has no member named ‘Next’; did you mean ‘exp’? ../../../wine/include/winnt.h:760:21: note: in definition of macro ‘CONTAINING_RECORD’ ../../../wine/dlls/rpcrt4/rpc_transport.c:1789:37: error: ‘RpcServerProtseq {aka struct _RpcServerProtseq}’ has no member named ‘conn’ ../../../wine/include/winnt.h:760:21: note: in definition of macro ‘CONTAINING_RECORD’ ../../../wine/dlls/rpcrt4/rpc_transport.c:1803:46: error: ‘RpcConnection {aka struct _RpcConnection}’ has no member named ‘Next’; did you mean ‘exp’? ../../../wine/include/winnt.h:760:21: note: in definition of macro ‘CONTAINING_RECORD’ ../../../wine/dlls/rpcrt4/rpc_transport.c: In function ‘rpcrt4_protseq_sock_wait_for_new_connection’: ../../../wine/dlls/rpcrt4/rpc_transport.c:1846:41: error: ‘RpcServerProtseq {aka struct _RpcServerProtseq}’ has no member named ‘conn’ ../../../wine/include/winnt.h:760:21: note: in definition of macro ‘CONTAINING_RECORD’ ../../../wine/dlls/rpcrt4/rpc_transport.c:1850:50: error: ‘RpcConnection {aka struct _RpcConnection}’ has no member named ‘Next’; did you mean ‘exp’? ../../../wine/include/winnt.h:760:21: note: in definition of macro ‘CONTAINING_RECORD’ Makefile:737: recipe for target 'rpc_transport.o' failed -- Alexandre Julliard julliard(a)winehq.org
On 15.05.2017 20:50, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/rpcrt4/rpc_binding.h | 2 +- dlls/rpcrt4/rpc_server.c | 18 ++++---- dlls/rpcrt4/rpc_server.h | 2 +- dlls/rpcrt4/rpc_transport.c | 110 +++++++++++++++----------------------------- 4 files changed, 50 insertions(+), 82 deletions(-) This breaks the Windows build:
Sorry about that, I will send a fixed version. However, this rises my other related concern. What do you think about changing this code to always use winsock? We'd use single implementation, making the code cleaner and avoiding breaking other configs. I guess the original reasoning was combination of performance and less mature winsock implementation at the time. I don't think winsock overhead would be too bad in this case and ws2_32 should be up to the task. I'm working on more changes to this area, so if we're going to switch to winsock, it would make sense to start with it. Thanks, Jacek
Jacek Caban <jacek(a)codeweavers.com> writes:
Sorry about that, I will send a fixed version. However, this rises my other related concern. What do you think about changing this code to always use winsock? We'd use single implementation, making the code cleaner and avoiding breaking other configs. I guess the original reasoning was combination of performance and less mature winsock implementation at the time. I don't think winsock overhead would be too bad in this case and ws2_32 should be up to the task.
I'm working on more changes to this area, so if we're going to switch to winsock, it would make sense to start with it.
If performance is reasonable, I have no objections. -- Alexandre Julliard julliard(a)winehq.org
On 15.05.2017 21:38, Alexandre Julliard wrote:
Jacek Caban <jacek(a)codeweavers.com> writes:
Sorry about that, I will send a fixed version. However, this rises my other related concern. What do you think about changing this code to always use winsock? We'd use single implementation, making the code cleaner and avoiding breaking other configs. I guess the original reasoning was combination of performance and less mature winsock implementation at the time. I don't think winsock overhead would be too bad in this case and ws2_32 should be up to the task.
I'm working on more changes to this area, so if we're going to switch to winsock, it would make sense to start with it. If performance is reasonable, I have no objections.
I did a bit of benchmarking. When doing a lot of trivial RPC calls in a loop using localhost, winsock based implementation (with the patch I just sent) is about 6% slower than Windows, although over 3 times slower than Wine with UNIX sockets. That a test that should show the biggest slow down, since no real networking is done. When I tried with real network RPC (which is more realistic scenario), it's only about 7% slower than UNIX sockets and 10% faster than Windows. Those numbers will vary mostly on connection used, but it's clear that its impact is an order of magnitude smaller than connection speed. Jacek
On Mon, May 15, 2017 at 4:27 PM, Jacek Caban <jacek(a)codeweavers.com> wrote:
... Sorry about that, I will send a fixed version. However, this rises my other related concern. What do you think about changing this code to always use winsock? We'd use single implementation, making the code cleaner and avoiding breaking other configs. I guess the original reasoning was combination of performance and less mature winsock implementation at the time. I don't think winsock overhead would be too bad in this case and ws2_32 should be up to the task.
I would vote for that, wininet was already changed and we had some discussion about changing winhttp too [1] but Hans was not amused with the idea ;-) Maybe this is a good time to discuss again (?). Having a single point of network input/output will simplify network debugging IMO. [1] http://wine.1045685.n8.nabble.com/Re-winhttp-Handle-EINTR-from-connect-and-p...
On Mon, 2017-05-15 at 16:39 -0300, Bruno Jesus wrote:
On Mon, May 15, 2017 at 4:27 PM, Jacek Caban <jacek(a)codeweavers.com> wrote:
... Sorry about that, I will send a fixed version. However, this rises my other related concern. What do you think about changing this code to always use winsock? We'd use single implementation, making the code cleaner and avoiding breaking other configs. I guess the original reasoning was combination of performance and less mature winsock implementation at the time. I don't think winsock overhead would be too bad in this case and ws2_32 should be up to the task.
I would vote for that, wininet was already changed and we had some discussion about changing winhttp too [1] but Hans was not amused with the idea ;-)
Yes, there was a strong argument for moving wininet: apps depending on winsock initialization. rpcrt4 has two implementations, one based on Unix sockets and one based on Windows sockets. It may make sense to have just the Windows version if performance is acceptable.
Maybe this is a good time to discuss again (?). Having a single point of network input/output will simplify network debugging IMO.
Right, because we just fixed the last winsock bug ;-)
On 15.05.2017 22:36, Hans Leidekker wrote:
On Mon, 2017-05-15 at 16:39 -0300, Bruno Jesus wrote:
On Mon, May 15, 2017 at 4:27 PM, Jacek Caban <jacek(a)codeweavers.com> wrote:
... Sorry about that, I will send a fixed version. However, this rises my other related concern. What do you think about changing this code to always use winsock? We'd use single implementation, making the code cleaner and avoiding breaking other configs. I guess the original reasoning was combination of performance and less mature winsock implementation at the time. I don't think winsock overhead would be too bad in this case and ws2_32 should be up to the task. I would vote for that, wininet was already changed and we had some discussion about changing winhttp too [1] but Hans was not amused with the idea ;-) Yes, there was a strong argument for moving wininet: apps depending on winsock initialization. rpcrt4 has two implementations, one based on Unix sockets and one based on Windows sockets. It may make sense to have just the Windows version if performance is acceptable.
winhttp also has two implementations, although it's hidden with preprocessor macros, so it's not as bad as in case of rpcrt4. Since we have winsock implementation for mingw builds in winhttp anyway, I don't see a reason to keep Unix socket implementation there. It just adds #ifdefs and all the compatibility code for no good reason. That said, in my opinion, it would be nice to use winsock in winhttp. I also think it's not very important through.
Maybe this is a good time to discuss again (?). Having a single point of network input/output will simplify network debugging IMO. Right, because we just fixed the last winsock bug ;-)
If there is a bug in winsock that winhttp would hit (and winhttp uses rather basic functionality, nothing tricky), I'd much rather have it fixed than worked around ;-) Jacek
participants (4)
-
Alexandre Julliard -
Bruno Jesus -
Hans Leidekker -
Jacek Caban