From cf4f2847c1a1df101e2983a0e1e8682ace323c0d Mon Sep 17 00:00:00 2001 From: Markus Goetz Date: Tue, 28 Sep 2010 18:32:16 +0200 Subject: QNAM: More zerocopy changes The zerocopy download buffer is now QSharedPointer instead of the QSharedPointer to QVarLengthArray. This will be a bit leaner to handle by QML and QtWebKit and does not tie us to a QVLA that much. Also fix some bugs related to signal emissions and the return value of bytesAvailable(). Now the behaviour should be the same if a zerocopy buffer is used or not. Reviewed-by: Peter Hartmann Reviewed-by: Thiago Macieira --- .../access/qhttpnetworkconnectionchannel.cpp | 3 +- src/network/access/qnetworkreplyimpl.cpp | 41 ++--- src/network/access/qnetworkreplyimpl_p.h | 9 +- tests/auto/qnetworkreply/tst_qnetworkreply.cpp | 178 ++++++++++++++++++++- .../access/qnetworkreply/tst_qnetworkreply.cpp | 7 +- 5 files changed, 209 insertions(+), 29 deletions(-) diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp index e39f9ed..617602d 100644 --- a/src/network/access/qhttpnetworkconnectionchannel.cpp +++ b/src/network/access/qhttpnetworkconnectionchannel.cpp @@ -427,7 +427,8 @@ void QHttpNetworkConnectionChannel::_q_receiveReply() replyPrivate->totalProgress += haveRead; // the user will get notified of it via progress signal - emit reply->dataReadProgress(replyPrivate->totalProgress, replyPrivate->bodyLength); + if (haveRead > 0) + emit reply->dataReadProgress(replyPrivate->totalProgress, replyPrivate->bodyLength); } else if (!replyPrivate->isChunked() && !replyPrivate->autoDecompress && replyPrivate->bodyLength > 0) { // bulk files like images should fulfill these properties and diff --git a/src/network/access/qnetworkreplyimpl.cpp b/src/network/access/qnetworkreplyimpl.cpp index 3a629cf..2dbd21c 100644 --- a/src/network/access/qnetworkreplyimpl.cpp +++ b/src/network/access/qnetworkreplyimpl.cpp @@ -49,16 +49,12 @@ #include "QtNetwork/qnetworksession.h" #include "qnetworkaccesshttpbackend_p.h" #include "qnetworkaccessmanager_p.h" -#include #include QT_BEGIN_NAMESPACE -typedef QSharedPointer > QVarLengthArraySharedPointer; -QT_END_NAMESPACE -Q_DECLARE_METATYPE(QVarLengthArraySharedPointer) -QT_BEGIN_NAMESPACE +Q_DECLARE_METATYPE(QSharedPointer) inline QNetworkReplyImplPrivate::QNetworkReplyImplPrivate() : backend(0), outgoingData(0), outgoingDataBuffer(0), @@ -69,7 +65,9 @@ inline QNetworkReplyImplPrivate::QNetworkReplyImplPrivate() httpStatusCode(0), state(Idle) , downloadBuffer(0) - , downloadBufferPosition(0) + , downloadBufferReadPosition(0) + , downloadBufferCurrentSize(0) + , downloadBufferMaximumSize(0) { } @@ -603,6 +601,11 @@ void QNetworkReplyImplPrivate::appendDownstreamData(const QByteArray &data) qFatal("QNetworkReplyImplPrivate::appendDownstreamData not implemented"); } +static void downloadBufferDeleter(char *ptr) +{ + delete[] ptr; +} + char* QNetworkReplyImplPrivate::getDownloadBuffer(qint64 size) { Q_Q(QNetworkReplyImpl); @@ -611,12 +614,12 @@ char* QNetworkReplyImplPrivate::getDownloadBuffer(qint64 size) if (!downloadBuffer) { QVariant bufferAllocationPolicy = request.attribute(QNetworkRequest::MaximumDownloadBufferSizeAttribute); if (bufferAllocationPolicy.isValid() && bufferAllocationPolicy.toLongLong() >= size) { - downloadBufferArray = QSharedPointer >(new QVarLengthArray()); - downloadBufferArray->reserve(size); - - downloadBuffer = downloadBufferArray->data(); + downloadBufferCurrentSize = 0; + downloadBufferMaximumSize = size; + downloadBuffer = new char[downloadBufferMaximumSize]; // throws if allocation fails + downloadBufferPointer = QSharedPointer(downloadBuffer, downloadBufferDeleter); - q->setAttribute(QNetworkRequest::DownloadBufferAttribute, qVariantFromValue > > (downloadBufferArray)); + q->setAttribute(QNetworkRequest::DownloadBufferAttribute, qVariantFromValue > (downloadBufferPointer)); } } @@ -644,12 +647,12 @@ void QNetworkReplyImplPrivate::appendDownstreamDataDownloadBuffer(qint64 bytesRe bytesDownloaded = bytesReceived; lastBytesDownloaded = bytesReceived; - // Update the array so our user (e.g. QtWebKit) knows the real size - if (bytesReceived > 0) - downloadBufferArray->resize(bytesReceived); + downloadBufferCurrentSize = bytesReceived; emit q->downloadProgress(bytesDownloaded, bytesTotal); - emit q->readyRead(); + // Only emit readyRead when actual data is there + if (bytesDownloaded > 0) + emit q->readyRead(); } void QNetworkReplyImplPrivate::finished() @@ -846,7 +849,7 @@ qint64 QNetworkReplyImpl::bytesAvailable() const // Special case for the "zero copy" download buffer Q_D(const QNetworkReplyImpl); if (d->downloadBuffer) { - qint64 maxAvail = d->downloadBufferArray->size() - d->downloadBufferPosition; + qint64 maxAvail = d->downloadBufferCurrentSize - d->downloadBufferReadPosition; return QNetworkReply::bytesAvailable() + maxAvail; } @@ -907,12 +910,12 @@ qint64 QNetworkReplyImpl::readData(char *data, qint64 maxlen) // Special case code if we have the "zero copy" download buffer if (d->downloadBuffer) { - qint64 maxAvail = qMin(d->downloadBufferArray->size() - d->downloadBufferPosition, maxlen); + qint64 maxAvail = qMin(d->downloadBufferCurrentSize - d->downloadBufferReadPosition, maxlen); if (maxAvail == 0) return d->state == QNetworkReplyImplPrivate::Finished ? -1 : 0; // FIXME what about "Aborted" state? - qMemCopy(data, d->downloadBuffer + d->downloadBufferPosition, maxAvail); - d->downloadBufferPosition += maxAvail; + qMemCopy(data, d->downloadBuffer + d->downloadBufferReadPosition, maxAvail); + d->downloadBufferReadPosition += maxAvail; return maxAvail; } diff --git a/src/network/access/qnetworkreplyimpl_p.h b/src/network/access/qnetworkreplyimpl_p.h index 2cb3082..e944601 100644 --- a/src/network/access/qnetworkreplyimpl_p.h +++ b/src/network/access/qnetworkreplyimpl_p.h @@ -62,7 +62,7 @@ #include "QtCore/qbuffer.h" #include "private/qringbuffer_p.h" #include "private/qbytedata_p.h" -#include +#include QT_BEGIN_NAMESPACE @@ -206,9 +206,12 @@ public: State state; // only used when the "zero copy" style is used. Else readBuffer is used. - QSharedPointer< QVarLengthArray > downloadBufferArray; + // Please note that the whole "zero copy" download buffer API is private right now. Do not use it. + qint64 downloadBufferReadPosition; + qint64 downloadBufferCurrentSize; + qint64 downloadBufferMaximumSize; + QSharedPointer downloadBufferPointer; char* downloadBuffer; - qint64 downloadBufferPosition; Q_DECLARE_PUBLIC(QNetworkReplyImpl) }; diff --git a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp index 2ad4060..f7f0519 100644 --- a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp @@ -75,8 +75,7 @@ #include "../network-settings.h" -typedef QSharedPointer > QVarLengthArraySharedPointer; -Q_DECLARE_METATYPE(QVarLengthArraySharedPointer) +Q_DECLARE_METATYPE(QSharedPointer) Q_DECLARE_METATYPE(QNetworkReply*) Q_DECLARE_METATYPE(QAuthenticator*) Q_DECLARE_METATYPE(QNetworkProxy) @@ -292,6 +291,8 @@ private Q_SLOTS: void getFromHttpIntoBuffer_data(); void getFromHttpIntoBuffer(); + void getFromHttpIntoBuffer2_data(); + void getFromHttpIntoBuffer2(); void ioGetFromHttpWithoutContentLength(); @@ -4297,6 +4298,7 @@ void tst_QNetworkReply::getFromHttpIntoBuffer_data() QTest::newRow("rfc-internal") << QUrl("http://" + QtNetworkSettings::serverName() + "/qtest/rfc3252.txt"); } +// Please note that the whole "zero copy" download buffer API is private right now. Do not use it. void tst_QNetworkReply::getFromHttpIntoBuffer() { QFETCH(QUrl, url); @@ -4319,9 +4321,11 @@ void tst_QNetworkReply::getFromHttpIntoBuffer() // Compare the memory buffer QVariant downloadBufferAttribute = reply->attribute(QNetworkRequest::DownloadBufferAttribute); + QVERIFY(downloadBufferAttribute.isValid()); + QSharedPointer sharedPointer = downloadBufferAttribute.value >(); bool memoryComparison = (0 == memcmp(static_cast(reference.readAll().data()), - downloadBufferAttribute.value > >()->constData(), reference.size())); + sharedPointer.data(), reference.size())); QVERIFY(memoryComparison); // Make sure the normal reading works @@ -4335,6 +4339,174 @@ void tst_QNetworkReply::getFromHttpIntoBuffer() QVERIFY(reply->atEnd()); } +// FIXME we really need to consolidate all those server implementations +class GetFromHttpIntoBuffer2Server : QObject { + Q_OBJECT; + qint64 dataSize; + qint64 dataSent; + QTcpServer server; + QTcpSocket *client; + bool serverSendsContentLength; + bool chunkedEncoding; + +public: + GetFromHttpIntoBuffer2Server (qint64 ds, bool sscl, bool ce) : dataSize(ds), dataSent(0), + client(0), serverSendsContentLength(sscl), chunkedEncoding(ce) { + server.listen(); + connect(&server, SIGNAL(newConnection()), this, SLOT(newConnectionSlot())); + } + + int serverPort() { + return server.serverPort(); + } + +public slots: + + void newConnectionSlot() { + client = server.nextPendingConnection(); + client->setParent(this); + connect(client, SIGNAL(readyRead()), this, SLOT(readyReadSlot())); + connect(client, SIGNAL(bytesWritten(qint64)), this, SLOT(bytesWrittenSlot(qint64))); + } + + void readyReadSlot() { + client->readAll(); + client->write("HTTP/1.0 200 OK\n"); + if (serverSendsContentLength) + client->write(QString("Content-Length: " + QString::number(dataSize) + "\n").toAscii()); + if (chunkedEncoding) + client->write(QString("Transfer-Encoding: chunked\n").toAscii()); + client->write("Connection: close\n\n"); + } + + void bytesWrittenSlot(qint64 amount) { + Q_UNUSED(amount); + if (dataSent == dataSize && client) { + // close eventually + + // chunked encoding: we have to send a last "empty" chunk + if (chunkedEncoding) + client->write(QString("0\r\n\r\n").toAscii()); + + client->disconnectFromHost(); + server.close(); + client = 0; + return; + } + + // send data + if (client && client->bytesToWrite() < 100*1024 && dataSent < dataSize) { + qint64 amount = qMin(qint64(16*1024), dataSize - dataSent); + QByteArray data(amount, '@'); + + if (chunkedEncoding) { + client->write(QString(QString("%1").arg(amount,0,16).toUpper() + "\r\n").toAscii()); + client->write(data.constData(), amount); + client->write(QString("\r\n").toAscii()); + } else { + client->write(data.constData(), amount); + } + + dataSent += amount; + } + } +}; + +class GetFromHttpIntoBuffer2Client : QObject { + Q_OBJECT +private: + bool useDownloadBuffer; + QNetworkReply *reply; + qint64 uploadSize; + QList bytesAvailableList; +public: + GetFromHttpIntoBuffer2Client (QNetworkReply *reply, bool useDownloadBuffer, qint64 uploadSize) + : useDownloadBuffer(useDownloadBuffer), reply(reply), uploadSize(uploadSize) + { + connect(reply, SIGNAL(metaDataChanged()), this, SLOT(metaDataChangedSlot())); + connect(reply, SIGNAL(readyRead()), this, SLOT(readyReadSlot())); + connect(reply, SIGNAL(finished()), this, SLOT(finishedSlot())); + } + + public slots: + void metaDataChangedSlot() { + if (useDownloadBuffer) { + QSharedPointer sharedPointer = qvariant_cast >(reply->attribute(QNetworkRequest::DownloadBufferAttribute)); + QVERIFY(!sharedPointer.isNull()); // It will be 0 if it failed + } + + // metaDataChanged needs to come before everything else + QVERIFY(bytesAvailableList.isEmpty()); + } + + void readyReadSlot() { + QVERIFY(!reply->isFinished()); + + qint64 bytesAvailable = reply->bytesAvailable(); + + // bytesAvailable must never be 0 + QVERIFY(bytesAvailable != 0); + + if (bytesAvailableList.length() < 5) { + // We assume that the first few times the bytes available must be less than the complete size, e.g. + // the bytesAvailable() function works correctly in case of a downloadBuffer. + QVERIFY(bytesAvailable < uploadSize); + } + if (!bytesAvailableList.isEmpty()) { + // Also check that the same bytesAvailable is not coming twice in a row + QVERIFY(bytesAvailableList.last() != bytesAvailable); + } + + bytesAvailableList.append(bytesAvailable); + // Add bytesAvailable to a list an parse + } + + void finishedSlot() { + // We should have already received all readyRead + QVERIFY(bytesAvailableList.last() == uploadSize); + } +}; + +void tst_QNetworkReply::getFromHttpIntoBuffer2_data() +{ + QTest::addColumn("useDownloadBuffer"); + + QTest::newRow("use-download-buffer") << true; + QTest::newRow("do-not-use-download-buffer") << false; +} + +// This test checks mostly that signal emissions are in correct order +// Please note that the whole "zero copy" download buffer API is private right now. Do not use it. +void tst_QNetworkReply::getFromHttpIntoBuffer2() +{ + QFETCH(bool, useDownloadBuffer); + + // On my Linux Desktop the results are already visible with 128 kB, however we use this to have good results. +#if defined(Q_OS_SYMBIAN) || defined(Q_WS_WINCE_WM) + // Show some mercy to non-desktop platform/s + enum {UploadSize = 4*1024*1024}; // 4 MB +#else + enum {UploadSize = 32*1024*1024}; // 32 MB +#endif + + GetFromHttpIntoBuffer2Server server(UploadSize, true, false); + + QNetworkRequest request(QUrl("http://127.0.0.1:" + QString::number(server.serverPort()) + "/?bare=1")); + if (useDownloadBuffer) + request.setAttribute(QNetworkRequest::MaximumDownloadBufferSizeAttribute, 1024*1024*128); // 128 MB is max allowed + + QNetworkAccessManager manager; + QNetworkReplyPtr reply = manager.get(request); + + GetFromHttpIntoBuffer2Client client(reply, useDownloadBuffer, UploadSize); + + connect(reply, SIGNAL(finished()), &QTestEventLoop::instance(), SLOT(exitLoop()), Qt::QueuedConnection); + QTestEventLoop::instance().enterLoop(40); + QCOMPARE(reply->error(), QNetworkReply::NoError); + QVERIFY(!QTestEventLoop::instance().timeout()); +} + + // Is handled somewhere else too, introduced this special test to have it more accessible void tst_QNetworkReply::ioGetFromHttpWithoutContentLength() { diff --git a/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp index 0098d8e..f75d681 100644 --- a/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/benchmarks/network/access/qnetworkreply/tst_qnetworkreply.cpp @@ -51,8 +51,7 @@ #include "../../../../auto/network-settings.h" -typedef QSharedPointer > QVarLengthArraySharedPointer; -Q_DECLARE_METATYPE(QVarLengthArraySharedPointer) +Q_DECLARE_METATYPE(QSharedPointer) class TimedSender: public QThread { @@ -661,6 +660,7 @@ private: bool useDownloadBuffer; QNetworkReply *reply; qint64 uploadSize; + QList bytesAvailableList; public: HttpDownloadPerformanceClientDownloadBuffer (QNetworkReply *reply, bool useDownloadBuffer, qint64 uploadSize) : useDownloadBuffer(useDownloadBuffer), reply(reply), uploadSize(uploadSize) @@ -672,7 +672,7 @@ public: void finishedSlot() { if (useDownloadBuffer) { QVariant downloadBufferAttribute = reply->attribute(QNetworkRequest::DownloadBufferAttribute); - QSharedPointer > data = downloadBufferAttribute.value > >(); + QSharedPointer data = downloadBufferAttribute.value >(); } else { // We did not have a download buffer but we still need to benchmark having the data, e.g. reading it all. char* replyData = (char*) qMalloc(uploadSize); @@ -690,6 +690,7 @@ void tst_qnetworkreply::httpDownloadPerformanceDownloadBuffer_data() QTest::newRow("do-not-use-download-buffer") << false; } +// Please note that the whole "zero copy" download buffer API is private right now. Do not use it. void tst_qnetworkreply::httpDownloadPerformanceDownloadBuffer() { QFETCH(bool, useDownloadBuffer); -- cgit v0.12