Bug 232220 – [CF] Reduce duplication and unneeded buffer allocations and copying in URL code, also remove unused methods and functions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

[CF] Reduce duplication and unneeded buffer allocations and copying, also remove unused methods and functions

[Darin Adler]

Created attachment 442322 [details]
Patch

[Darin Adler]

Created attachment 442323 [details]
Patch

[Darin Adler]

Created attachment 442741 [details]
Patch

[Darin Adler]

Created attachment 442745 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84859824>

[Darin Adler]

This 2-week-old patch (from before my week of vacation) is looking good and I think it can get reviewed and landed now.

[Alex Christensen]

Comment on attachment 442745 [details]
Patch

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

> Source/WTF/wtf/cf/CFURLExtras.cpp:37
> +    auto buffer = static_cast<uint8_t*>(malloc(bytesLength));

Is it possible to use fastMalloc and then make a CFAllocator that calls fastFree?

> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:646
> +    encoder << IPC::DataReference(bytesAsVector(url));

It seems like this could be even further optimized in the future because we don't need the bytes in a vector.  We just need the bytes to go into the encoder.

[Darin Adler]

Comment on attachment 442745 [details]
Patch

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

I’ll look into both of your suggestions, Alex, for a follow-up.

>> Source/WTF/wtf/cf/CFURLExtras.cpp:37
>> +    auto buffer = static_cast<uint8_t*>(malloc(bytesLength));
> 
> Is it possible to use fastMalloc and then make a CFAllocator that calls fastFree?

Note that this function is only used in the originalURLData function, which has been using malloc/realloc until now, so the use of malloc here is not a change.

And, yes, that *is* possible.

I’m not sure if it’s worth it.

We’d have to store the CFAllocatorRef in a global, and it would be kind of a lot more code, and the CFURL itself is stored in something allocated by malloc, not fastMalloc.

>> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:646
>> +    encoder << IPC::DataReference(bytesAsVector(url));
> 
> It seems like this could be even further optimized in the future because we don't need the bytes in a vector.  We just need the bytes to go into the encoder.

Yes; would be a lot more code but could be good.

Note that in non-pathological cases there is no fastMalloc/Free here because the vector has inline capacity. So an optimized implementation would save a copy, but there is not the epic slowness implied by the way this code looks.

[Darin Adler]

Comment on attachment 442745 [details]
Patch

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

>>> Source/WTF/wtf/cf/CFURLExtras.cpp:37
>>> +    auto buffer = static_cast<uint8_t*>(malloc(bytesLength));
>> 
>> Is it possible to use fastMalloc and then make a CFAllocator that calls fastFree?
> 
> Note that this function is only used in the originalURLData function, which has been using malloc/realloc until now, so the use of malloc here is not a change.
> 
> And, yes, that *is* possible.
> 
> I’m not sure if it’s worth it.
> 
> We’d have to store the CFAllocatorRef in a global, and it would be kind of a lot more code, and the CFURL itself is stored in something allocated by malloc, not fastMalloc.

Occurs to me that we could also maybe use the fastMalloc CFAllocator for the CFData object itself, not just the data payload. Again, not sure how valuable, but we could probably use that all over the place in WebKit when calling CF.

[Darin Adler]

Committed r285536 (244053@main): <https://commits.webkit.org/244053@main>

[Ryan Haddad]

It looks like this change has caused some API tests to crash:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.CoreFoundation            0x00007fff31381c71 CFURLGetBytes + 43
1   com.apple.JavaScriptCore            0x0000000101cc263f WTF::bytesAsString(__CFURL const*) + 31 (CFURLExtras.cpp:46)
2   com.apple.WebKit                    0x0000000103e0a51e -[WKBrowsingContextController loadHTMLString:baseURL:userData:] + 168 (WKBrowsingContextController.mm:176)
3   TestWebKitAPI                       0x0000000100f5ee46 WebKit2UserContentTest_RemoveAllUserScripts_Test::TestBody() + 422 (UserContentTest.mm:241)
4   TestWebKitAPI                       0x0000000101070f5c void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 72
5   TestWebKitAPI                       0x0000000101070ec5 testing::Test::Run() + 193
6   TestWebKitAPI                       0x0000000101071be6 testing::TestInfo::Run() + 240
7   TestWebKitAPI                       0x00000001010723a9 testing::TestSuite::Run() + 303
8   TestWebKitAPI                       0x000000010107d1c4 testing::internal::UnitTestImpl::RunAllTests() + 840
9   TestWebKitAPI                       0x000000010107cd65 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 72
10  TestWebKitAPI                       0x000000010107ccf0 testing::UnitTest::Run() + 108
11  TestWebKitAPI                       0x0000000100f2ebf8 TestWebKitAPI::TestsController::run(int, char**) + 120 (TestsController.cpp:90)
12  TestWebKitAPI                       0x000000010104906f main + 378
13  libdyld.dylib                       0x00007fff6b3f9cc9 start + 1

https://build.webkit.org/#/builders/13/builds/6631/steps/17/logs/stdio

[Darin Adler]

But not in EWS? OK, please revert the change.

[Darin Adler]

Looks like it did fail in EWS and I ignored it :-(

[Alex Christensen]

It was all green when I reviewed it.  Something strange is going on with EWS.

[Ryan Haddad]

The result from https://ews-build.webkit.org/#/builders/3/builds/59830 came back 1 hour ago, perhaps it hadn't completed by the time it was reviewed and landed?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 232915

[Darin Adler]

Committed r285588 (244096@main): <https://commits.webkit.org/244096@main>

[Darin Adler]

Comment on attachment 442745 [details]
Patch

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

>>> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:646
>>> +    encoder << IPC::DataReference(bytesAsVector(url));
>> 
>> It seems like this could be even further optimized in the future because we don't need the bytes in a vector.  We just need the bytes to go into the encoder.
> 
> Yes; would be a lot more code but could be good.
> 
> Note that in non-pathological cases there is no fastMalloc/Free here because the vector has inline capacity. So an optimized implementation would save a copy, but there is not the epic slowness implied by the way this code looks.

After further exploration this isn’t practical.

The CFURLGetBytes function returns all the bytes in a provided buffer, and the Encoder abstraction does not allow clients to first get a pointer and then put bytes there, so we can’t easily do better than the Vector with inline capacity.