Bug 198957

Summary:

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

Product:

WebKit

Reporter:

Fujii Hironori <fujii.hironori>

Component:

Web Template Framework

Assignee:

Fujii Hironori <fujii.hironori>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, basuke, beidson, cdumez, darin, ews-watchlist, ggaren, mitz, rniwa, sam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=133968

Attachments:

Description	Flags
WIP patch	none
Patch	none
Patch for landing	none

Fujii Hironori

Reported 2019-06-18 00:27:48 PDT

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&&).

Attachments
WIP patch (4.76 KB, patch) 2019-06-18 00:29 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Patch (8.87 KB, patch) 2019-06-18 02:28 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Patch for landing (12.06 KB, patch) 2019-06-18 20:28 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Fujii Hironori

Comment 1 2019-06-18 00:29:37 PDT

Created attachment 372330 [details] WIP patch

EWS Watchlist

Comment 2 2019-06-18 00:33:05 PDT

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

Comment 3 2019-06-18 02:28:08 PDT

Created attachment 372332 [details] Patch

Alex Christensen

Comment 4 2019-06-18 14:49:00 PDT

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 5 2019-06-18 18:25:54 PDT

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 6 2019-06-18 20:24:27 PDT

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

Comment 7 2019-06-18 20:28:18 PDT

Created attachment 372422 [details] Patch for landing

Fujii Hironori

Comment 8 2019-06-19 17:51:59 PDT

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

Radar WebKit Bug Importer

Comment 9 2019-06-19 17:52:12 PDT

<rdar://problem/51922449>

Note You need to log in before you can comment on or make changes to this bug.