From 37a0e9c180e6dd670b5ba722218df00d88d8ad3b Mon Sep 17 00:00:00 2001 From: Alex Trotsenko Date: Fri, 25 Mar 2016 18:30:35 +0200 Subject: [PATCH] QNetworkReply: remove double buffering Since 5.7 we can write downloaded byte arrays directly to the QIODevice's internal read buffer without any memcpy/alloc overhead. This makes various other buffers redundant, so remove them. Task-number: QTBUG-45926 Change-Id: I577af70f856c9b852b7569a0070ae2bcbb4faaae Reviewed-by: Oswald Buddenhagen Reviewed-by: Edward Welbourne Reviewed-by: Markus Goetz (Woboq GmbH) --- src/network/access/qnetworkreplyhttpimpl.cpp | 46 +++++++------------- src/network/access/qnetworkreplyhttpimpl_p.h | 5 +-- src/network/access/qnetworkreplyimpl.cpp | 40 +++++------------ src/network/access/qnetworkreplyimpl_p.h | 5 +-- 4 files changed, 29 insertions(+), 67 deletions(-) diff --git a/src/network/access/qnetworkreplyhttpimpl.cpp b/src/network/access/qnetworkreplyhttpimpl.cpp index 5c403250ab..6bf3c56db2 100644 --- a/src/network/access/qnetworkreplyhttpimpl.cpp +++ b/src/network/access/qnetworkreplyhttpimpl.cpp @@ -296,7 +296,7 @@ qint64 QNetworkReplyHttpImpl::bytesAvailable() const // if we load from cache device if (d->cacheLoadDevice) { - return QNetworkReply::bytesAvailable() + d->cacheLoadDevice->bytesAvailable() + d->downloadMultiBuffer.byteAmount(); + return QNetworkReply::bytesAvailable() + d->cacheLoadDevice->bytesAvailable(); } // zerocopy buffer @@ -305,7 +305,7 @@ qint64 QNetworkReplyHttpImpl::bytesAvailable() const } // normal buffer - return QNetworkReply::bytesAvailable() + d->downloadMultiBuffer.byteAmount(); + return QNetworkReply::bytesAvailable(); } bool QNetworkReplyHttpImpl::isSequential () const @@ -329,12 +329,6 @@ qint64 QNetworkReplyHttpImpl::readData(char* data, qint64 maxlen) if (d->cacheLoadDevice) { // FIXME bytesdownloaded, position etc? - // There is something already in the buffer we buffered before because the user did not read() - // anything, so we read there first: - if (!d->downloadMultiBuffer.isEmpty()) { - return d->downloadMultiBuffer.read(data, maxlen); - } - qint64 ret = d->cacheLoadDevice->read(data, maxlen); return ret; } @@ -351,25 +345,14 @@ qint64 QNetworkReplyHttpImpl::readData(char* data, qint64 maxlen) } // normal buffer - if (d->downloadMultiBuffer.isEmpty()) { - if (d->state == d->Finished || d->state == d->Aborted) - return -1; - return 0; - } + if (d->state == d->Finished || d->state == d->Aborted) + return -1; - if (maxlen == 1) { - // optimization for getChar() - *data = d->downloadMultiBuffer.getChar(); - if (readBufferSize()) - emit readBufferFreed(1); - return 1; - } - - maxlen = qMin(maxlen, d->downloadMultiBuffer.byteAmount()); - qint64 bytesRead = d->downloadMultiBuffer.read(data, maxlen); + qint64 wasBuffered = d->bytesBuffered; + d->bytesBuffered = 0; if (readBufferSize()) - emit readBufferFreed(bytesRead); - return bytesRead; + emit readBufferFreed(wasBuffered); + return 0; } void QNetworkReplyHttpImpl::setReadBufferSize(qint64 size) @@ -387,12 +370,12 @@ bool QNetworkReplyHttpImpl::canReadLine () const return true; if (d->cacheLoadDevice) - return d->cacheLoadDevice->canReadLine() || d->downloadMultiBuffer.canReadLine(); + return d->cacheLoadDevice->canReadLine(); if (d->downloadZerocopyBuffer) return memchr(d->downloadZerocopyBuffer + d->downloadBufferReadPosition, '\n', d->downloadBufferCurrentSize - d->downloadBufferReadPosition); - return d->downloadMultiBuffer.canReadLine(); + return false; } #ifndef QT_NO_SSL @@ -444,6 +427,7 @@ QNetworkReplyHttpImplPrivate::QNetworkReplyHttpImplPrivate() , resumeOffset(0) , preMigrationDownloaded(-1) , bytesDownloaded(0) + , bytesBuffered(0) , downloadBufferReadPosition(0) , downloadBufferCurrentSize(0) , downloadZerocopyBuffer(0) @@ -1054,10 +1038,11 @@ void QNetworkReplyHttpImplPrivate::replyDownloadData(QByteArray d) cacheSaveDevice->write(item.constData(), item.size()); if (!isHttpRedirectResponse()) - downloadMultiBuffer.append(item); + buffer.append(item); bytesWritten += item.size(); } + bytesBuffered += bytesWritten; pendingDownloadDataCopy.clear(); QVariant totalSize = cookedHeaders.value(QNetworkRequest::ContentLengthHeader); @@ -1830,9 +1815,8 @@ void QNetworkReplyHttpImplPrivate::_q_cacheLoadReadyRead() // If there are still bytes available in the cacheLoadDevice then the user did not read // in response to the readyRead() signal. This means we have to load from the cacheLoadDevice // and buffer that stuff. This is needed to be able to properly emit finished() later. - while (cacheLoadDevice->bytesAvailable() && !isHttpRedirectResponse()) { - downloadMultiBuffer.append(cacheLoadDevice->readAll()); - } + while (cacheLoadDevice->bytesAvailable() && !isHttpRedirectResponse()) + buffer.append(cacheLoadDevice->readAll()); if (cacheLoadDevice->isSequential()) { // check if end and we can read the EOF -1 diff --git a/src/network/access/qnetworkreplyhttpimpl_p.h b/src/network/access/qnetworkreplyhttpimpl_p.h index 669258f15b..4aba915c7d 100644 --- a/src/network/access/qnetworkreplyhttpimpl_p.h +++ b/src/network/access/qnetworkreplyhttpimpl_p.h @@ -240,12 +240,11 @@ public: quint64 resumeOffset; qint64 preMigrationDownloaded; - // Used for normal downloading. For "zero copy" the downloadZerocopyBuffer is used - QByteDataBuffer downloadMultiBuffer; QByteDataBuffer pendingDownloadData; // For signal compression qint64 bytesDownloaded; + qint64 bytesBuffered; - // only used when the "zero copy" style is used. Else downloadMultiBuffer is used. + // Only used when the "zero copy" style is used. // Please note that the whole "zero copy" download buffer API is private right now. Do not use it. qint64 downloadBufferReadPosition; qint64 downloadBufferCurrentSize; diff --git a/src/network/access/qnetworkreplyimpl.cpp b/src/network/access/qnetworkreplyimpl.cpp index d69d5983cb..54930a351a 100644 --- a/src/network/access/qnetworkreplyimpl.cpp +++ b/src/network/access/qnetworkreplyimpl.cpp @@ -184,17 +184,13 @@ void QNetworkReplyImplPrivate::_q_copyReadyRead() break; bytesToRead = qBound(1, bytesToRead, copyDevice->bytesAvailable()); - QByteArray byteData; - byteData.resize(bytesToRead); - qint64 bytesActuallyRead = copyDevice->read(byteData.data(), byteData.size()); + qint64 bytesActuallyRead = copyDevice->read(buffer.reserve(bytesToRead), bytesToRead); if (bytesActuallyRead == -1) { - byteData.clear(); + buffer.chop(bytesToRead); backendNotify(NotifyCopyFinished); break; } - - byteData.resize(bytesActuallyRead); - readBuffer.append(byteData); + buffer.chop(bytesToRead - bytesActuallyRead); if (!copyDevice->isSequential() && copyDevice->atEnd()) { backendNotify(NotifyCopyFinished); @@ -582,7 +578,7 @@ qint64 QNetworkReplyImplPrivate::nextDownstreamBlockSize() const if (readBufferMaxSize == 0) return DesiredBufferSize; - return qMax(0, readBufferMaxSize - readBuffer.byteAmount()); + return qMax(0, readBufferMaxSize - buffer.size()); } void QNetworkReplyImplPrivate::initCacheSaveDevice() @@ -624,7 +620,7 @@ void QNetworkReplyImplPrivate::initCacheSaveDevice() } // we received downstream data and send this to the cache -// and to our readBuffer (which in turn gets read by the user of QNetworkReply) +// and to our buffer (which in turn gets read by the user of QNetworkReply) void QNetworkReplyImplPrivate::appendDownstreamData(QByteDataBuffer &data) { Q_Q(QNetworkReplyImpl); @@ -641,7 +637,7 @@ void QNetworkReplyImplPrivate::appendDownstreamData(QByteDataBuffer &data) if (cacheSaveDevice) cacheSaveDevice->write(item.constData(), item.size()); - readBuffer.append(item); + buffer.append(item); bytesWritten += item.size(); } @@ -975,13 +971,6 @@ void QNetworkReplyImpl::close() d->finished(); } -bool QNetworkReplyImpl::canReadLine () const -{ - Q_D(const QNetworkReplyImpl); - return QNetworkReply::canReadLine() || d->readBuffer.canReadLine(); -} - - /*! Returns the number of bytes available for reading with QIODevice::read(). The number of bytes available may grow until @@ -996,14 +985,14 @@ qint64 QNetworkReplyImpl::bytesAvailable() const return QNetworkReply::bytesAvailable() + maxAvail; } - return QNetworkReply::bytesAvailable() + d_func()->readBuffer.byteAmount(); + return QNetworkReply::bytesAvailable(); } void QNetworkReplyImpl::setReadBufferSize(qint64 size) { Q_D(QNetworkReplyImpl); if (size > d->readBufferMaxSize && - size > d->readBuffer.byteAmount()) + size > d->buffer.size()) d->backendNotify(QNetworkReplyImplPrivate::NotifyDownstreamReadyWrite); QNetworkReply::setReadBufferSize(size); @@ -1061,19 +1050,12 @@ qint64 QNetworkReplyImpl::readData(char *data, qint64 maxlen) } - if (d->readBuffer.isEmpty()) - return d->state == QNetworkReplyPrivate::Finished ? -1 : 0; // FIXME what about "Aborted" state? + if (d->state == QNetworkReplyPrivate::Finished) + return -1; d->backendNotify(QNetworkReplyImplPrivate::NotifyDownstreamReadyWrite); - if (maxlen == 1) { - // optimization for getChar() - *data = d->readBuffer.getChar(); - return 1; - } - - maxlen = qMin(maxlen, d->readBuffer.byteAmount()); - return d->readBuffer.read(data, maxlen); + return 0; } /*! diff --git a/src/network/access/qnetworkreplyimpl_p.h b/src/network/access/qnetworkreplyimpl_p.h index 156cb411c7..054cbcc3a7 100644 --- a/src/network/access/qnetworkreplyimpl_p.h +++ b/src/network/access/qnetworkreplyimpl_p.h @@ -81,7 +81,6 @@ public: virtual void close() Q_DECL_OVERRIDE; virtual qint64 bytesAvailable() const Q_DECL_OVERRIDE; virtual void setReadBufferSize(qint64 size) Q_DECL_OVERRIDE; - virtual bool canReadLine () const Q_DECL_OVERRIDE; virtual qint64 readData(char *data, qint64 maxlen) Q_DECL_OVERRIDE; virtual bool event(QEvent *) Q_DECL_OVERRIDE; @@ -187,8 +186,6 @@ public: QList proxyList; #endif - // Used for normal downloading. For "zero copy" the downloadBuffer is used - QByteDataBuffer readBuffer; qint64 bytesDownloaded; qint64 lastBytesDownloaded; qint64 bytesUploaded; @@ -199,7 +196,7 @@ public: State state; - // only used when the "zero copy" style is used. Else readBuffer is used. + // Only used when the "zero copy" style is used. // Please note that the whole "zero copy" download buffer API is private right now. Do not use it. qint64 downloadBufferReadPosition; qint64 downloadBufferCurrentSize;