Bug 102306 – For single element arrays use the pointer into the CFDataRef instead of copying data

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

This is an optimization when USE(NETWORK_CFDATA_ARRAY_CALLBACK) is available. Generally, WebCore copies the data returned from CFNetwork into its own buffers once the resource is done loading. However, if the resource is cached, CFNetwork will return us a CFArrayRef with exactly 1 CFDataRef in it. In such a case, it might be beneficial to not copy the data and just use the byte pointer i.e. call CFDataGetBytePtr() and use that memory directly.

[Pratik Solanki]

<rdar://problem/12267471>

[Pratik Solanki]

Created attachment 174487 [details]
Patch

[Pratik Solanki]

Comment on attachment 174487 [details]
Patch

Retracting. This needs another tweak.

[Pratik Solanki]

Created attachment 174569 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 174569 [details]
Patch

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

Looks reasonable to me. Brady might want to have a look too, as he's been refactoring adjacent code.

> Source/WebCore/ChangeLog:22
> +        (WebCore):

It is helpful to edit ChangeLogs to remove useless gunk like this. ChangeLogs are meant for human consumption, so anything you can do to make reading easier is a plus (but obviously, not hugely important for somethign trivial like this).

> Source/WebCore/platform/SharedBuffer.cpp:149
> +    const char *buffer = singleDataArrayBuffer();

Star goes to the left.

> Source/WebCore/platform/cf/SharedBufferCF.cpp:144
> +        return reinterpret_cast<const char *>(CFDataGetBytePtr(m_dataArray.at(0).get()));

Ditto.

[Pratik Solanki]

Thanks for the review. I'll commit with the changes suggested. I also had to change a variable name since the compiler got confused about buffer being a const char * and a member function.

[Pratik Solanki]

Committed r134987: <http://trac.webkit.org/changeset/134987>

[Darin Adler]

Comment on attachment 174569 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:151
> +    const char *buffer = singleDataArrayBuffer();
> +    if (buffer)
> +        return buffer;

This would be slightly nicer with the variable definition inside the if.

> Source/WebCore/platform/cf/SharedBufferCF.cpp:143
> +    if (m_dataArray.size() == 1)

This would be slightly nicer as an early return.

[Darin Adler]

(In reply to comment #6)
> I also had to change a variable name since the compiler got confused about buffer being a const char * and a member function.

An alternate solution in cases like that is to use this->functionName syntax to call a member function that is shadowed by a same-named local variable. We like that because it lets us use the cleanest simplest names for both.

[Pratik Solanki]

Reopening to attach new patch.

[Pratik Solanki]

Created attachment 174785 [details]
Be slightly nicer!

Sounds good. Added a patch with the comments addressed

[Brent Fulgham]

Comment on attachment 174785 [details]
Be slightly nicer!

Since the Apple guys are on vacation, and you have executed the changes ap and Darin asked for, r=me.

[WebKit Review Bot]

Comment on attachment 174785 [details]
Be slightly nicer!

Clearing flags on attachment: 174785

Committed r135221: <http://trac.webkit.org/changeset/135221>

[Pratik Solanki]

Brent, thanks for the r+ and cq+. Marking this bug as fixed.