Implement buffering in the DataStream class, to improve performance on Windows.
ClosedPublic

Authored by dfaure on Apr 28 2020, 6:49 PM.

Details

Summary

The internal QWindowsPipeWriter in Qt doesn't do any buffering, so the first 8
bytes (the tag) were sent first, and the rest only later.

This solution uses an explicit flush() method, while doing the flushing
in the destructor would have been much more convenient and less
error-prone, but the flushing can throw an exception, so we need
to do it inside the try/catch -- and certainly not when the stream
is deleted because another exception happened. This would all be
so much simpler without the use of exceptions :-)

Test Plan

all tests pass (on Linux)

Diff Detail

Repository
R165 Akonadi
Branch
2020_04_implement_buffering_in_DataStream_for_Windows
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26047
Build 26065: arc lint + arc unit
dfaure created this revision.Apr 28 2020, 6:49 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 28 2020, 6:49 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
dfaure requested review of this revision.Apr 28 2020, 6:49 PM

I'm having second thoughts, this probably doesn't make a big difference. Packet fragmentation doesn't have a big impact compared to fetching stuff from the internet. In Kevin Funk's testcase (fetching lots of objects from a server) it certainly makes no difference, the overall time is similar on Windows (with the fragmentation) and on Linux (without).

dvratil added inline comments.Apr 30 2020, 8:52 AM
src/private/datastream_p.cpp
45

Why don't you just handle the exception in the destructor?

dfaure added inline comments.Apr 30 2020, 9:24 AM
src/private/datastream_p.cpp
45

Hmm, and do what with it? The calling code knows what to do with the exception, here we don't.

In fact that's the same as just folding flush() into the destructor, and just not throwing an exception at all.

But the question remains, won't the caller need to know. Let's see....

src/server/connection.h
164

This code here throws ProtocolException (see also line 167), so that e.g. slotSendHello() disconnects the socket. Or in handleIncomingData, when sendResponse throws, we set m_connectionClosing to true in order to break from the loop and disconnect the socket.

If we eat the exception in ~DataStream none of that will happen. Isn't that a problem?

src/server/notificationsubscriber.cpp
688

ok here it's just a warning, we can do that.

dvratil accepted this revision.May 2 2020, 9:33 PM

Ok.

Would switching back to TCP on Windows (using QTcpSocket) solve this issue without the explicit flush? I'm also playing around with replacing the entire Qt socket code by ZeroMQ that apparently works pretty well on both Windows and Linux, so we could drop all the non-blocking wait-for-data socket code on Windows...

This revision is now accepted and ready to land.May 2 2020, 9:33 PM
dfaure closed this revision.May 3 2020, 8:01 AM