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 <oswald.buddenhagen@theqtcompany.com>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Reviewed-by: Markus Goetz (Woboq GmbH) <markus@woboq.com>

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<qint64>(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<qint64>(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<qint64>(0, readBufferMaxSize - readBuffer.byteAmount());
+    return qMax<qint64>(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<qint64>(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<QNetworkProxy> 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;