Bug 147908 - Always use a byte-sized lock implementation
Summary: Always use a byte-sized lock implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 147841
  Show dependency treegraph
 
Reported: 2015-08-11 16:10 PDT by Filip Pizlo
Modified: 2015-08-11 21:21 PDT (History)
13 users (show)

See Also:


Attachments
probably works (57.86 KB, patch)
2015-08-11 16:13 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (64.40 KB, patch)
2015-08-11 16:22 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (64.35 KB, patch)
2015-08-11 16:33 PDT, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff
patch for landing (64.92 KB, patch)
2015-08-11 20:14 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-08-11 16:10:36 PDT
At time of writing, you could choose between Lock, which is a sizeof(void*) lock implementation with some nice theoretical properties, and ByteLock, which is a sizeof(uint8_t) lock implementation that empirically performs as well or better.

Clearly, we should make Lock use ByteLock's superior algorithm and get rid of ByteLock.
Comment 1 Filip Pizlo 2015-08-11 16:13:01 PDT
Created attachment 258774 [details]
probably works
Comment 2 Filip Pizlo 2015-08-11 16:22:58 PDT
Created attachment 258776 [details]
the patch
Comment 3 Filip Pizlo 2015-08-11 16:33:40 PDT
Created attachment 258778 [details]
the patch
Comment 4 Geoffrey Garen 2015-08-11 16:43:08 PDT
Comment on attachment 258778 [details]
the patch

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

r=me

> Source/WTF/ChangeLog:27
> +        - Move the algorithm in Lock into files called BasicLock.h|cpp. Make ParkingLot use
> +          BasicLock.

"Basic" doesn't say a lot to me about when to use or not use this lock. "Basic" feels good, but I probably shouldn't feel good about using this lock.

How about "ParkingLock". It helps to say "only the ParkingLot should use this".

Or, "WordLock" to describe a part of the tradeoff: bigger than a byte lock, so I probably don't want it.
Comment 5 Filip Pizlo 2015-08-11 20:08:13 PDT
(In reply to comment #4)
> Comment on attachment 258778 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258778&action=review
> 
> r=me
> 
> > Source/WTF/ChangeLog:27
> > +        - Move the algorithm in Lock into files called BasicLock.h|cpp. Make ParkingLot use
> > +          BasicLock.
> 
> "Basic" doesn't say a lot to me about when to use or not use this lock.
> "Basic" feels good, but I probably shouldn't feel good about using this lock.
> 
> How about "ParkingLock". It helps to say "only the ParkingLot should use
> this".
> 
> Or, "WordLock" to describe a part of the tradeoff: bigger than a byte lock,
> so I probably don't want it.

I'll call it WordLock.
Comment 6 Filip Pizlo 2015-08-11 20:14:57 PDT
Created attachment 258798 [details]
patch for landing
Comment 7 WebKit Commit Bot 2015-08-11 20:15:58 PDT
Attachment 258798 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/LockSpeedTest.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Filip Pizlo 2015-08-11 20:22:45 PDT
(In reply to comment #7)
> Attachment 258798 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WTF/benchmarks/LockSpeedTest.cpp:29:  Alphabetical sorting
> problem.  [build/include_order] [4]
> Total errors found: 1 in 14 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Fixed locally.
Comment 9 Filip Pizlo 2015-08-11 21:21:08 PDT
Landed in http://trac.webkit.org/changeset/188323