Bug 174907 - Update lock benchmarks
Summary: Update lock benchmarks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-27 15:11 PDT by JF Bastien
Modified: 2017-07-28 16:23 PDT (History)
8 users (show)

See Also:


Attachments
patch (1.43 KB, patch)
2017-07-27 15:12 PDT, JF Bastien
fpizlo: review-
Details | Formatted Diff | Diff
patch (12.01 KB, patch)
2017-07-27 18:20 PDT, JF Bastien
fpizlo: review+
fpizlo: commit-queue+
Details | Formatted Diff | Diff
patch (11.99 KB, patch)
2017-07-27 18:26 PDT, JF Bastien
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (11.99 KB, patch)
2017-07-27 18:29 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2017-07-27 15:11:29 PDT
It's just not there anymore.
Comment 1 JF Bastien 2017-07-27 15:12:32 PDT
Created attachment 316575 [details]
patch
Comment 2 Filip Pizlo 2017-07-27 15:13:42 PDT
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.
Comment 3 JF Bastien 2017-07-27 18:16:38 PDT
I'm updating all the benchmarks now.
Comment 4 JF Bastien 2017-07-27 18:20:06 PDT
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.
Comment 5 Build Bot 2017-07-27 18:22:03 PDT
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 6 Filip Pizlo 2017-07-27 18:24:11 PDT
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.
Comment 7 JF Bastien 2017-07-27 18:26:54 PDT
Created attachment 316590 [details]
patch

Fix indentation.
Comment 8 JF Bastien 2017-07-27 18:27:30 PDT
(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 9 WebKit Commit Bot 2017-07-27 18:27:46 PDT
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
Comment 10 JF Bastien 2017-07-27 18:29:55 PDT
Created attachment 316592 [details]
patch

Fix oops.
Comment 11 WebKit Commit Bot 2017-07-27 19:17:16 PDT
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 12 WebKit Commit Bot 2017-07-27 19:17:40 PDT
Comment on attachment 316592 [details]
patch

Clearing flags on attachment: 316592

Committed r219988: <http://trac.webkit.org/changeset/219988>
Comment 13 WebKit Commit Bot 2017-07-27 19:17:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2017-07-28 16:21:58 PDT
<rdar://problem/33601358>
Comment 15 Radar WebKit Bug Importer 2017-07-28 16:23:06 PDT
<rdar://problem/33601363>