[PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.

[Sebastian Lackner]

On 24.10.2016 15:46, Jacek Caban wrote:
> On 20.10.2016 19:17, Sebastian Lackner wrote:
>> On 19.10.2016 19:05, Jacek Caban wrote:
>>> v2: Don't store async queues in a list.
>>>
>>> Signed-off-by: Jacek Caban <jacek at codeweavers.com>
>>> ---
>>>  server/async.c  | 34 +++++++++++++++++++++++++++++++++
>>>  server/device.c | 58
>>> +++++++++++++++++++++++++--------------------------------
>>>  server/file.h   | 14 ++++++++++++++
>>>  3 files changed, 73 insertions(+), 33 deletions(-)
>>>
>> I'm not sure if Alexandre has already agreed to this approach, but from my
>> point of view it would make sense to do some measurements. How fast is the
>> proposed named pipe approach compared to alternatives (unpatched Wine, or
>> the Linux-only implementation included in Wine Staging)?
> 
> As Alexandre said, I think we should concentrate on correctness first,
> but I did some benchmarking that should give us overview.

That is correct, but of course there isn't just one way to implement named
pipes. It might also be possible to implement it in a portable way without
going through wineserver or using Linux-specific functions. It would require
more complexity on the ntdll side though.

In fact, having object handling in ntdll (at least as fast-path for process-
local objects) is something people have been thinking about since a long time,
and it probably could also be used to solve the message mode problem.
Nevertheless, I have to admit that reusing the driver functions is also very
elegant. ;)

> Proper
> measuring depends on many things and different implementations have
> various weaknesses. Here are a few things to keep in mind:
> 
> - My patchset leaves room for improvements as explained in [1]. I have
> prototyped patches for those optimizations and tested both with and
> without them.
> 
> - Calls with immediate results have very different characteristics than
> ones that return immediately. If we need to wait, then server round
> trips either using overlapped calls or blocking select require server
> round trips, for which extra data that needs to be transferred with my
> patchset (if not too big), shouldn't cause significant difference. When
> using my tests with large buffer, most calls should have immediate
> results, so I expect it to tests worst-case scenario.
> 
> - Current Wine implementation uses blocking socket calls for
> non-overlapped pipes. This significantly affects results, so I tested
> both in overlapped and non-overlapped mode (although calls themselves
> are not overlapped).
> 
> - Small buffer sizes would be interesting to test, because they cause
> mode blocking and thus would show differences in such case. Sadly,
> staging implementation doesn't take that into account, so results would
> be meaningless.
> 
> I used the attached test with parameters:
> npbench.exe 1024 10000 100000 (for non-overlapped)
> npbench.exe 1024 10000 100000 1 (for overlapped)
> 
> Results are also attached. Proper benchmarking would require a lot more
> care, but I think it shows what you asked for. This implementation is
> 3.3 times slower in overlapped case, 7.4 times slower in non-overlapped
> tests (3.1 and 2.5 times compared to Windows in VM). I'd say that's not
> too bad. Overlapped case is even faster than I expected.
> 
> BTW, did you expect staging patches to be so much faster in overlapped
> case than plain Wine? I didn't look too deeply in the code, maybe there
> is some optimization, but that's a bit suspicious.

Thanks, these are exactly the stats I was looking for. I'm not aware of
any special optimizations, so not sure how Staging can be faster.
If you used a prebuilt Staging version with compiler optimization enabled
that could also be the reason. Nevertheless, the values for
"plain wine+msgmode patchset+optimizations" actually look pretty good
when taking into account the wineserver roundtrip.

> 
>> In general I like the idea, but I fear that adding significant optimizations
>> afterwards will be very difficult.
> 
> FWIW, it should be fairly easy to rebase staging patches on top of my
> series. It leaves the whole previous implementation mostly untouched for
> byte mode case. That, however, may not be true after further changes in
> the future, esp. if we decided to use server I/O implementation for byte
> mode case as well.

Well, I would probably keep it for some more time in Staging, but I don't
think it would make sense to merge it into the development branch. Issues
are easier to debug when it behaves the same on all platforms.

BTW; I assume you are aware of it already, but your current implementation
lacks handling of PIPE_NOWAIT, which is also implemented in the Staging
patchset.

Best regards,
Sebastian