Bug 50082 – [Qt] Use QNetworkAccessManager zerocopy feature from QNetworkReplyHandler

• Status: RESOLVED
• Resolution: INVALID
• Component: Page Loading

[Markus Goetz]

(will attach the patch tomorrow)

[Markus Goetz]

Created attachment 74914 [details]
patch attempt 1

[Early Warning System Bot]

Attachment 74914 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6315052

[Markus Goetz]

Created attachment 74922 [details]
patch attempt 2

[Kenneth Rohde Christiansen]

Comment on attachment 74922 [details]
patch attempt 2

View in context: https://bugs.webkit.org/attachment.cgi?id=74922&action=review

> WebCore/platform/SharedBuffer.cpp:242
> -#if !PLATFORM(CF) || PLATFORM(QT)
> +#if !PLATFORM(CF) && !PLATFORM(QT)

this wont break normal build?

> WebCore/platform/SharedBuffer.h:45
>  #else
>  class NSData;
>  #endif
> +#endif

This added endif here seems wrong

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:238
> +    m_request.setAttribute(gMaximumDownloadBufferSizeAttribute, 1024*256); // 256 kB

needs spaces around *. Need dot at end of comment

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:370
> +    // Check if there is a zerocopy buffer

Needs dot at end of comment

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:378
> +        // Old school way. Use QNetworkReply as a QIODevice

Strange comment, rewrite also add dot at the end.

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:380
> +        m_usingZeroCopy = false;

maybe call this m_usingZeroCopyFeature

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:485
> +    // don't emit the "Document has moved here" type of HTML

End with dot, starts with capital

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:503
> +                           oldSize, bytesReceived - oldSize, bytesReceived - oldSize /*FixMe*/);

call it FIXME: plus add a description on WHAT to fix!

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:638
> +    // For zero copy

Misses dot.

> WebCore/platform/network/qt/QNetworkReplyHandler.h:94
> +    // for zerocopy. Holds the download data:

fix comment

> WebCore/platform/network/qt/QNetworkReplyHandler.h:96
> +    // for zerocopy it wraps m_byteBlock, otherwise it holds data normally:

also

> WebCore/platform/qt/QByteBlock.cpp:29
> +    QByteBlock* bb = new QByteBlock;

write out bb

> WebCore/platform/qt/QByteBlock.cpp:46
> +    return (const char*)(m_data.data());

I do not think the latter () are needed

> WebCore/platform/qt/QByteBlock.cpp:51
> +    m_size = s;

just write out s

> WebCore/platform/qt/QByteBlock.cpp:56
> +    m_capacity = c;

and c!

[Markus Goetz]

Created attachment 75372 [details]
v3

[Kenneth Rohde Christiansen]

Comment on attachment 75372 [details]
v3

View in context: https://bugs.webkit.org/attachment.cgi?id=75372&action=review

> WebCore/platform/SharedBuffer.h:45
>  #endif
> +#endif

I trust you that this is git showing the diff wrong... if not, you need to fix it

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:237
> +    // Then this buffer is provided to us in the QNetworkReply

Where is the dot!

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:240
> +
> +

No need for two newlines here

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:376
> +        m_usingZeroCopy = true;

Please rename m_usingZeroCopy to m_usingZeroCopyFeature

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:503
> +                           oldSize, bytesReceived - oldSize, bytesReceived - oldSize);

I would keep oldSize on the line above

> WebCore/platform/network/qt/ResourceHandleQt.cpp:180
> -    return false;
> +    return true;

Shouldnt there be a "if usingZeroCopyFeature" here?

> WebCore/platform/qt/QByteBlock.cpp:46
> +    return (const char*)(m_data.data());

Remove latter ()

> WebCore/platform/qt/SharedBufferQt.cpp:50
> +    SharedBuffer* sb = new SharedBuffer();

Shouldn't this be RefPtr<SharedBuffer*> sb = new SharedBuffer() ?

[Markus Goetz]

Comment on attachment 75372 [details]
v3

View in context: https://bugs.webkit.org/attachment.cgi?id=75372&action=review

Uploading new patch in a minute:)

>> WebCore/platform/SharedBuffer.h:45
>> +#endif
> 
> I trust you that this is git showing the diff wrong... if not, you need to fix it

Actually it is not showing it wrong. Just unintuitive. check the whole context.

>> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:237
>> +    // Then this buffer is provided to us in the QNetworkReply
> 
> Where is the dot!

argh

>> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:376
>> +        m_usingZeroCopy = true;
> 
> Please rename m_usingZeroCopy to m_usingZeroCopyFeature

No, with this patch we are always using the zero copy feature.
What we are checking is if the server allowed us to do it by giving a content-length header etc.

>> WebCore/platform/network/qt/ResourceHandleQt.cpp:180
>> +    return true;
> 
> Shouldnt there be a "if usingZeroCopyFeature" here?

No, it's always buffered by QNetworkReplyHandler now.
It's just different ways of buffering depending if zerocopy could be used or not. See QNetworkReplyHandler::sendResponseIfNeeded()

>> WebCore/platform/qt/SharedBufferQt.cpp:50
>> +    SharedBuffer* sb = new SharedBuffer();
> 
> Shouldn't this be RefPtr<SharedBuffer*> sb = new SharedBuffer() ?

I copied/modified that from somewhere else and as far as I understand the refcounting it should be fine..

[Markus Goetz]

Created attachment 75383 [details]
v4

[Kenneth Rohde Christiansen]

Comment on attachment 75383 [details]
v4

View in context: https://bugs.webkit.org/attachment.cgi?id=75383&action=review

> WebCore/WebCore.gypi:3144
> +            'platform/qt/QByteBlock.cpp',
> +            'platform/qt/QByteBlock.h',

Why is this called QByteBlock? ByteBlockQt seems more in line what we have elsewhere

[Holger Freyther]

(In reply to comment #9)
> (From update of attachment 75383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75383&action=review
> 
> > WebCore/WebCore.gypi:3144
> > +            'platform/qt/QByteBlock.cpp',
> > +            'platform/qt/QByteBlock.h',
> 
> Why is this called QByteBlock? ByteBlockQt seems more in line what we have elsewhere

I am sorry that we always nitpick so much in WebKit. I have to admit that both names have issues. ByteBlockQt indicate that it is some generic interface and this is the Qt specific part and QByteBlock has the danger of clashing with future versions of Qt.  :}

I think for now you can remove the 'capacity' call as no one is calling it?

I also have some questions. So QNetworkAccessManager will internally start changing the QSharedDataPointer<char>? Does this work with realloc? Somehow I would assume to see something like a QList<QByteArray> but I don't. So Qt will still re-alloc internally?

[Markus Goetz]

(In reply to comment #10)
> I am sorry that we always nitpick so much in WebKit. I have to admit that both names have issues. ByteBlockQt indicate that it is some generic interface and this is the Qt specific part and QByteBlock has the danger of clashing with future versions of Qt.  :}

Yeah, well, but this one will always have the WebCore namespace :)

> I think for now you can remove the 'capacity' call as no one is calling it?

I might use it later with the image decoders, let's see.

> 
> I also have some questions. So QNetworkAccessManager will internally start changing the QSharedDataPointer<char>? Does this work with realloc? Somehow I would assume to see something like a QList<QByteArray> but I don't. So Qt will still re-alloc internally?

It will not re-allocate.
It goes through this codepath only if you allow it to do so (in the QNetworkRequest) and then only if the server supports it (e.g. content-length header is there). In my measurements 70% of the bytes coming with normal websurfing have that condition.
The QList<QByteArray> thing (actually not that but similar) is used internally in the other 30% of the cases.

Thanks for comments! :)

[Kenneth Rohde Christiansen]

This is why we should call it WebCore::QtByteBlock, this follows our current way of doing things

[Jocelyn Turcotte]

Created attachment 76367 [details]
v4 (rebased)

Here is the rebased patch, feel free to modify it or redo it from scratch before resending it for review.

I couldn't test the patch completely since, with Qt 4.7.1, I always got a null pointer from m_reply->attribute(QNetworkRequest::DownloadBufferAttribute). I got the same result with the v4 non-rebased patch so this might be a bug in Qt. You might want to verify the patch with a correct behaving Qt or you'll get a crash if I made any mistake rebasing that code-path.

[Kenneth Rohde Christiansen]

Comment on attachment 76367 [details]
v4 (rebased)

Oh come on Jocelyn :-) won't you do that QByteBlock to QtByteBlock renaming?

[Alexis Menard (darktears)]

Comment on attachment 76367 [details]
v4 (rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=76367&action=review

Could you please rebase again your changes? The source tree was reorganized some times ago. Thanks

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:59
> +#if QT_VERSION >= QT_VERSION_CHECK(4, 7, 0)

We do now :).

[Markus Goetz]

Created attachment 84725 [details]
one more patch attempt

[WebKit Review Bot]

Attachment 84725 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:484:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 84725 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8094034

[Markus Goetz]

Created attachment 84739 [details]
..and one more, this time with the new files

[Kenneth Rohde Christiansen]

Comment on attachment 84739 [details]
..and one more, this time with the new files

View in context: https://bugs.webkit.org/attachment.cgi?id=84739&action=review

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:231
> +    // Then this buffer is provided to us in the QNetworkReply.

I don't understand this English

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:382
> +       m_usingZeroCopy = true;

Wrong identation

[Andreas Kling]

Committed r80551: <http://trac.webkit.org/changeset/80551>

[WebKit Review Bot]

http://trac.webkit.org/changeset/80551 might have broken Qt Windows 32-bit Release and Qt Windows 32-bit Debug

[Csaba Osztrogonác]

(In reply to comment #21)
> Committed r80551: <http://trac.webkit.org/changeset/80551>

Reopen, because it was rolled out: http://trac.webkit.org/changeset/80560

[Markus Goetz]

Not much news from me here, got no time to work on this these days :(
If no one else takes it over I'll hope I can do it before Qt 4.8 comes out..

[Siddharth Mathur]

Any volunteers ? :)

[Luiz Agostini]

I will take a look.