Bug 148089

Summary:

Replace all remaining uses of WTF::Mutex with WTF::Lock

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

Web Template Framework

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, basile_clement, benjamin, buildbot, commit-queue, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, rniwa, sam

Priority:

Version:

Other

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

147841

Attachments:

Description	Flags
work in progress	none
the patch	buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks	none
Archive of layout-test-results from ews106 for mac-mavericks-wk2	none
the patch	none
the patch	none
the patch	none
the patch	none
the patch	none
the patch	none
fix even more builds	none
the patch	ggaren: review+

Filip Pizlo

Reported 2015-08-17 13:27:33 PDT

And replace WTF::ThreadCondition with WTF::Condition.

Attachments
work in progress (188.09 KB, patch) 2015-08-17 14:45 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (198.26 KB, patch) 2015-08-17 16:03 PDT, Filip Pizlo	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews100 for mac-mavericks (759.99 KB, application/zip) 2015-08-17 16:52 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (780.96 KB, application/zip) 2015-08-17 16:57 PDT, Build Bot	no flags	Details
the patch (210.12 KB, patch) 2015-08-17 20:37 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (210.86 KB, patch) 2015-08-17 21:18 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (211.21 KB, patch) 2015-08-17 21:33 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (212.28 KB, patch) 2015-08-17 22:15 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (212.70 KB, patch) 2015-08-17 22:35 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (212.87 KB, patch) 2015-08-17 22:41 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
fix even more builds (213.26 KB, patch) 2015-08-17 23:18 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (213.70 KB, patch) 2015-08-17 23:46 PDT, Filip Pizlo	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2015-08-17 14:45:56 PDT

Created attachment 259190 [details] work in progress

Filip Pizlo

Comment 2 2015-08-17 16:03:10 PDT

Created attachment 259197 [details] the patch I plan to add some more things to this patch, like some better tests and benchmarks for the new Condition functionality. But right now this is passing all of our existing tests, which are pretty comprehensive.

Build Bot

Comment 3 2015-08-17 16:52:21 PDT

Comment on attachment 259197 [details] the patch Attachment 259197 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/69504 Number of test failures exceeded the failure limit.

Build Bot

Comment 4 2015-08-17 16:52:25 PDT

Created attachment 259207 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Build Bot

Comment 5 2015-08-17 16:57:37 PDT

Comment on attachment 259197 [details] the patch Attachment 259197 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/69508 Number of test failures exceeded the failure limit.

Build Bot

Comment 6 2015-08-17 16:57:41 PDT

Created attachment 259210 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Filip Pizlo

Comment 7 2015-08-17 19:21:02 PDT

Comment on attachment 259197 [details] the patch Clearing r? because EWS reports test crashes

Filip Pizlo

Comment 8 2015-08-17 19:26:34 PDT

(In reply to comment #7) > Comment on attachment 259197 [details] > the patch > > Clearing r? because EWS reports test crashes Fixed locally. It was obvious when I tested debug.

Filip Pizlo

Comment 9 2015-08-17 20:37:24 PDT

Created attachment 259232 [details] the patch Fixes crashes in the last patch. Adds a speed test.

WebKit Commit Bot

Comment 10 2015-08-17 20:39:18 PDT

Attachment 259232 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 11 2015-08-17 21:18:08 PDT

Created attachment 259236 [details] the patch Fix some builds.

WebKit Commit Bot

Comment 12 2015-08-17 21:20:57 PDT

Attachment 259236 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 13 2015-08-17 21:33:25 PDT

Created attachment 259239 [details] the patch Another build fix.

WebKit Commit Bot

Comment 14 2015-08-17 21:34:56 PDT

Attachment 259239 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 15 2015-08-17 22:15:06 PDT

Created attachment 259246 [details] the patch More build fixes for Linux, iOS, and Windows.

WebKit Commit Bot

Comment 16 2015-08-17 22:17:56 PDT

Attachment 259246 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 13 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 17 2015-08-17 22:35:39 PDT

Created attachment 259249 [details] the patch More build fixes.

WebKit Commit Bot

Comment 18 2015-08-17 22:38:12 PDT

Attachment 259249 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 19 2015-08-17 22:41:58 PDT

Created attachment 259250 [details] the patch Fix more things

WebKit Commit Bot

Comment 20 2015-08-17 22:45:31 PDT

Attachment 259250 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 21 2015-08-17 23:18:35 PDT

Created attachment 259252 [details] fix even more builds

WebKit Commit Bot

Comment 22 2015-08-17 23:20:26 PDT

Attachment 259252 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 23 2015-08-17 23:46:27 PDT

Created attachment 259254 [details] the patch Bunch more Windows fixes.

WebKit Commit Bot

Comment 24 2015-08-17 23:47:56 PDT

Attachment 259254 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:130: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:205: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:210: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:214: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:222: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:225: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:230: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/benchmarks/ConditionSpeedTest.cpp:234: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 12 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Geoffrey Garen

Comment 25 2015-08-18 10:14:53 PDT

Comment on attachment 259254 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=259254&action=review r=me > Source/WTF/wtf/Condition.h:66 > + // May wait too long on some OSes: the C++ standard library implementation of > + // std::condition_variable::wait_until(), which this uses internally, may convert the absolute > + // time to an absolute time according to the system clock, which isn't monotonic. It will do this > + // so that it can use pthread_cond_timedwait(). Depending on the OS and if an unfortunate > + // combination of thread interleavings and system time changes happens, this may cause waitUntil() > + // to wait longer than it's supposed to. However, it's typical for OSes to turn the absolute > + // timeout in pthread_cond_timedwait() to a relative timeout before calling into the kernel. This > + // is what glibc does, for example. This seems more like documentation of an OS bug than of an intended quirk of the design here. Perhaps it doesn't belong here.

Filip Pizlo

Comment 26 2015-08-18 12:20:10 PDT

(In reply to comment #25) > Comment on attachment 259254 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259254&action=review > > r=me > > > Source/WTF/wtf/Condition.h:66 > > + // May wait too long on some OSes: the C++ standard library implementation of > > + // std::condition_variable::wait_until(), which this uses internally, may convert the absolute > > + // time to an absolute time according to the system clock, which isn't monotonic. It will do this > > + // so that it can use pthread_cond_timedwait(). Depending on the OS and if an unfortunate > > + // combination of thread interleavings and system time changes happens, this may cause waitUntil() > > + // to wait longer than it's supposed to. However, it's typical for OSes to turn the absolute > > + // timeout in pthread_cond_timedwait() to a relative timeout before calling into the kernel. This > > + // is what glibc does, for example. > > This seems more like documentation of an OS bug than of an intended quirk of > the design here. Perhaps it doesn't belong here. Fair enough, removed!

Filip Pizlo

Comment 27 2015-08-18 12:32:09 PDT

Landed in http://trac.webkit.org/changeset/188594

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