Bug 198957 – Add WTF::crossThreadCopy(T&&) to utilize String::isolatedCopy() &&

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Fujii Hironori]

Add WTF::crossThreadCopy(T&&) to utilize String::isolatedCopy() &&

&&-qualified String::isolatedCopy() has a optimization which
just does WTFMove if it isSafeToSendToAnotherThread which means
the object hasOneRef.
https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/text/WTFString.cpp?rev=246531#L640

WTF::crossThreadCopy is using only &-qualified isolatedCopy.
To use the optimization, add WTF::crossThreadCopy(T&&).

[Fujii Hironori]

Created attachment 372330 [details]
WIP patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 2 in 3 files


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

[Fujii Hironori]

Created attachment 372332 [details]
Patch

[Alex Christensen]

Comment on attachment 372332 [details]
Patch

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

Great optimization.  r=me

> Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:48
> +    // moved-out RefPtr becomes nullptr

This comment isn't needed.
Could you also add a test that has two references to original, moves one into crossThreadCopy, and verifies that an isolated copy is indeed made?

> Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:66
> +    // moved-out RefPtr becomes nullptr

Ditto, unnecessary comment.

[Fujii Hironori]

Comment on attachment 372332 [details]
Patch

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

To actually improve WebKit, we have more tasks:
1. Replace crossThreadCopy(something) → crossThreadCopy(WTFMove(something)) if possible
2. Add &&-qualified isolatedCopy method to some classes.
Volunteers are welcome.

>> Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:48
>> +    // moved-out RefPtr becomes nullptr
> 
> This comment isn't needed.
> Could you also add a test that has two references to original, moves one into crossThreadCopy, and verifies that an isolated copy is indeed made?

It's a good idea. Will do.

[Fujii Hironori]

Comment on attachment 372332 [details]
Patch

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

> Source/WTF/wtf/CrossThreadCopier.h:158
> +    static Type copy(Type&& source)

I can use a forwarding reference for this by fixing CrossThreadTask issue. Will fix.

Just for the record, here is the compilation error of CrossThreadTask I met.

> [4/808] Building CXX object Tools\TestWebKitAPI\CMakeFiles\TestWTFLib.dir\Tests\WTF\CrossThreadTask.cpp.obj
> FAILED: Tools/TestWebKitAPI/CMakeFiles/TestWTFLib.dir/Tests/WTF/CrossThreadTask.cpp.obj
> C:\PROGRA~1\LLVM\bin\clang-cl.exe  /nologo -TP -DBUILDING_TestWTF -DBUILDING_WITH_CMAKE=1 -DGTEST_CREATE_SHARED_LIBRARY=0 -DGTEST_HAS_PTHREAD=0 -DGTEST_HAS_RTTI=0 -DHAVE_CONFIG_H=1 -DNOMINMAX -DTestWTFLib_EXPORTS -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WIN32_WINNT=0x601 -D_WINDOWS -D_WINSOCKAPI_="" -I. -I..\..\Tools\TestWebKitAPI -I..\..\Source\ThirdParty\gtest\include -IWTF\Headers -IDerivedSources -I..\..\Source\ThirdParty -I..\..\WebKitLibraries\win\include /W4 -fdiagnostics-color=always -fcolor-diagnostics -Wno-parentheses-equality -Wno-noexcept-type -Qunused-arguments -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-unknown-argument -Wno-nonportable-include-path -Wno-unknown-pragmas -Wno-macro-redefined -Wno-undef /DWIN32 /D_WINDOWS  /GR- /EHsc- -fno-strict-aliasing /MD /Zi /Ob0 /Od /RTC1   /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646 /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /std:c++17 /utf-8 /validate-charset -fmsc-version=1911 /showIncludes /FoTools\TestWebKitAPI\CMakeFiles\TestWTFLib.dir\Tests\WTF\CrossThreadTask.cpp.obj /FdTools\TestWebKitAPI\CMakeFiles\TestWTFLib.dir\ -c ..\..\Tools\TestWebKitAPI\Tests\WTF\CrossThreadTask.cpp
> In file included from ..\..\Tools\TestWebKitAPI\Tests\WTF\CrossThreadTask.cpp:28:
> WTF\Headers\wtf/CrossThreadTask.h(70,65): error: no matching function for call to 'crossThreadCopy'
>     return CrossThreadTask([method, arguments = std::make_tuple(crossThreadCopy<Arguments>(arguments)...)]() mutable {
>                                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~
> ..\..\Tools\TestWebKitAPI\Tests\WTF\CrossThreadTask.cpp(111,21): note: in instantiation of function template specialization 'WTF::createCrossThreadTask<const TestWebKitAPI::LifetimeLogger &, const TestWebKitAPI::LifetimeLogger &, const TestWebKitAPI::LifetimeLogger &, TestWebKitAPI::LifetimeLogger, TestWebKitAPI::LifetimeLogger, TestWebKitAPI::LifetimeLogger>' requested here
>         auto task = createCrossThreadTask(testFunction, logger1, logger2, logger3);
>                     ^
> WTF\Headers\wtf/CrossThreadCopier.h(166,27): note: candidate function template not viable: 1st argument ('const TestWebKitAPI::LifetimeLogger') would lose const qualifier
> template<typename T> auto crossThreadCopy(T&& source)
>                           ^
> 1 error generated.

[Fujii Hironori]

Created attachment 372422 [details]
Patch for landing

[Fujii Hironori]

Committed r246623: <https://trac.webkit.org/changeset/246623>

[Radar WebKit Bug Importer]

<rdar://problem/51922449>