Bug 174907

Created attachment 316575 [details] patch

Comment on attachment 316575 [details] patch I think the proper fix is to introduce a pthread_mutex-based lock, so that we have one in the test.

I'm updating all the benchmarks now.

Created attachment 316588 [details] patch Update to use new APIs, fix command-line, use std::mutex instead of deleted Mutex, and add unfair lock if available.

Attachment 316588 [details] did not pass style-queue: ERROR: Source/WTF/benchmarks/ToyLocks.h:476: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4] ERROR: Source/WTF/benchmarks/ToyLocks.h:476: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:477: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:478: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:478: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4] ERROR: Source/WTF/benchmarks/ToyLocks.h:479: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:480: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:481: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:482: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:483: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:484: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:485: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:486: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:487: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/benchmarks/ToyLocks.h:488: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 15 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 316588 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=316588&action=review I think this is OK to land, but it would have been nicer if you hadn't used std:: types. > Source/WTF/benchmarks/ConditionSpeedTest.cpp:76 > -template<typename Functor, typename ConditionType, typename LockType> > -void wait(ConditionType& condition, LockType& lock, const Functor& predicate) > + template<typename Functor, typename ConditionType, typename LockType, typename std::enable_if<!std::is_same<ConditionType, std::condition_variable>::value>::type* = nullptr> > + void wait(ConditionType& condition, LockType& lock, std::unique_lock<LockType>&, const Functor& predicate) > { > while (!predicate()) > condition.wait(lock); > } > > +template<typename Functor, typename ConditionType, typename LockType, typename std::enable_if<std::is_same<ConditionType, std::condition_variable>::value>::type* = nullptr> > +void wait(ConditionType& condition, LockType&, std::unique_lock<LockType>& locker, const Functor& predicate) > +{ > + while (!predicate()) > + condition.wait(locker); > +} > + This is really gross. You wouldn't have had to do this if you wrote your own SystemMutex/SystemCondition that wrapped pthread_mutex_t/pthread_cond_t.

Created attachment 316590 [details] patch Fix indentation.

(In reply to Filip Pizlo from comment #6) > Comment on attachment 316588 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316588&action=review > > I think this is OK to land, but it would have been nicer if you hadn't used > std:: types. > > > Source/WTF/benchmarks/ConditionSpeedTest.cpp:76 > > -template<typename Functor, typename ConditionType, typename LockType> > > -void wait(ConditionType& condition, LockType& lock, const Functor& predicate) > > + template<typename Functor, typename ConditionType, typename LockType, typename std::enable_if<!std::is_same<ConditionType, std::condition_variable>::value>::type* = nullptr> > > + void wait(ConditionType& condition, LockType& lock, std::unique_lock<LockType>&, const Functor& predicate) > > { > > while (!predicate()) > > condition.wait(lock); > > } > > > > +template<typename Functor, typename ConditionType, typename LockType, typename std::enable_if<std::is_same<ConditionType, std::condition_variable>::value>::type* = nullptr> > > +void wait(ConditionType& condition, LockType&, std::unique_lock<LockType>& locker, const Functor& predicate) > > +{ > > + while (!predicate()) > > + condition.wait(locker); > > +} > > + > > This is really gross. You wouldn't have had to do this if you wrote your > own SystemMutex/SystemCondition that wrapped pthread_mutex_t/pthread_cond_t. True, I wanted to test "whatever C++ gives us" though :)

Comment on attachment 316590 [details] patch Rejecting attachment 316590 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 316590, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WTF/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/4199436

Created attachment 316592 [details] patch Fix oops.

The commit-queue encountered the following flaky tests while processing attachment 316592 [details]: media/modern-media-controls/seek-backward-support/seek-backward-support.html bug 174916 (authors: graouts@apple.com, mcatanzaro@igalia.com, and ryanhaddad@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 316592 [details] patch Clearing flags on attachment: 316592 Committed r219988: <http://trac.webkit.org/changeset/219988>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33601358>

<rdar://problem/33601363>