Bug 117458 – XMLHttpRequest performs too many copies for ArrayBuffer results

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

[Ryosuke Niwa]

Consider merging https://chromium.googlesource.com/chromium/blink/+/bed266aa5a43f7c080c87e527bd35e2b80ecc7b7

This cuts
- two memsets (in ArrayBuffer::create and SharedBuffer::m_buffer::resize)
- one copy (SharedBuffer::m_buffer to ArrayBufferContents::m_data)
- one allocation (SharedBuffer::m_buffer)

Copy for consolidation still happens from m_segments to m_data instead of from
m_segments to m_buffer.

Quick localhost benchmark showed ~10% speedup.

[Alexey Proskuryakov]

Sounds like a good idea, but the patch cannot be landed as is, at least because of incorrect naming (getAsArrayBuffer is not a proper name for a getter).

[Wojciech Bielawski]

Does anyone working on it? If not I will.

[Wojciech Bielawski]

Created attachment 222975 [details]
Blink's implementation

[Sergio Villar Senin]

Comment on attachment 222975 [details]
Blink's implementation 

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

A couple of nits

> Source/JavaScriptCore/ChangeLog:7
> +

Although it's nice to refer to the source of the change, the ChangeLog should include a description of the patch at least including the savings we get.

> Source/WebCore/platform/SharedBuffer.cpp:295
> +    return arrayBuffer;

I think it's a good idea to use arrayBuffer.release() here.

[Wojciech Bielawski]

Created attachment 222978 [details]
Blink's implementation

[Alexey Proskuryakov]

Comment on attachment 222978 [details]
Blink's implementation

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

> Source/JavaScriptCore/runtime/ArrayBuffer.h:99
> +    // Only for use by Uint8ClampedArray::createUninitialized and SharedBuffer::getAsArrayBuffer.

getAsArrayBuffer is mentioned in this comment, and in two ChangeLogs, but this function no longer exists.

[Alexey Proskuryakov]

How did you decide that it's fine to override what's been requested in this comment?

-    // Only for use by Uint8ClampedArray::createUninitialized.
+    // Only for use by Uint8ClampedArray::createUninitialized and SharedBuffer::getAsArrayBuffer.

[Alexey Proskuryakov]

I understand that you need some way to merge the data into a buffer, but I don't know the rationale for this comment. Seems like whoever added it wanted us to not add more exceptions even in cases where we thought that we needed them.

[Wojciech Bielawski]

(In reply to comment #7)
> How did you decide that it's fine to override what's been requested in this comment?
> 
> -    // Only for use by Uint8ClampedArray::createUninitialized.
> +    // Only for use by Uint8ClampedArray::createUninitialized and SharedBuffer::getAsArrayBuffer.

It was in original commit. Shall I remove it?

[Wojciech Bielawski]

Created attachment 223087 [details]
Blink's implementation

[Wojciech Bielawski]

(In reply to comment #8)
> I understand that you need some way to merge the data into a buffer, but I don't know the rationale for this comment. Seems like whoever added it wanted us to not add more exceptions even in cases where we thought that we needed them.

I assume that the author of original commit didn't find any inconsistency in creaateUninitialized code reusage. This comment also seems like justification does it neccessary at all. I could remove that comment at all since creaateUninitialized is public anyway.

[Filip Pizlo]

Comment on attachment 223087 [details]
Blink's implementation

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

LGTM.  Did you mean to mark this as r?

> Source/JavaScriptCore/runtime/ArrayBuffer.h:99
> -    // Only for use by Uint8ClampedArray::createUninitialized.
> +    // Only for use by Uint8ClampedArray::createUninitialized and SharedBuffer::createArrayBuffer.

I agree with this change.  The reason why createUninitialized() is public is because it was impractical to use friends because of the template madness going on in Uint8ClamperArray.  This comment is a kind of unenforced "friend" replacement.

> Source/WebCore/platform/SharedBuffer.cpp:289
> +        ASSERT_NOT_REACHED();

I prefer RELEASE_ASSERT_NOT_REACHED().

[Alexey Proskuryakov]

Comment on attachment 223087 [details]
Blink's implementation

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

Nice!

> Source/WebCore/platform/SharedBuffer.cpp:279
> +    RefPtr<ArrayBuffer> arrayBuffer = ArrayBuffer::createUninitialized(static_cast<unsigned>(size()), 1);

Nit: sizeof(char) would make this code slightly easier to read - with "1", most people will need to open another file to check what createUninitialized() arguments are.

>> Source/WebCore/platform/SharedBuffer.cpp:289
>> +        ASSERT_NOT_REACHED();
> 
> I prefer RELEASE_ASSERT_NOT_REACHED().

We don't have a lot of these in WebCore, and I actually don't think that we should. Unlike in JSC, many logic errors don't cause huge unintended consequences such as security bugs, and it's better to have a web site with some functionality broken than a WebProcess crash.

In this particular case, the failure is pretty benign - we essentially fail the load, which could happen for other reasons, like a network failure.

[Wojciech Bielawski]

Created attachment 223223 [details]
Blink's implementation

[WebKit Commit Bot]

Comment on attachment 223223 [details]
Blink's implementation

Clearing flags on attachment: 223223

Committed r163444: <http://trac.webkit.org/changeset/163444>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.