Bug 147908

Summary: Always use a byte-sized lock implementation
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: Web Template FrameworkAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, basile_clement, benjamin, commit-queue, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, saam, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 147841    
Attachments:
Description Flags
probably works
none
the patch
none
the patch
ggaren: review+
patch for landing none

Filip Pizlo
Reported 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.
Attachments
probably works (57.86 KB, patch)
2015-08-11 16:13 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (64.40 KB, patch)
2015-08-11 16:22 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (64.35 KB, patch)
2015-08-11 16:33 PDT, Filip Pizlo 		ggaren: review+
DetailsFormatted DiffDiff
patch for landing (64.92 KB, patch)
2015-08-11 20:14 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2015-08-11 16:13:01 PDT
Created attachment 258774 [details] probably works
Filip Pizlo
Comment 2 2015-08-11 16:22:58 PDT
Created attachment 258776 [details] the patch
Filip Pizlo
Comment 3 2015-08-11 16:33:40 PDT
Created attachment 258778 [details] the patch
Geoffrey Garen
Comment 4 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.
Filip Pizlo
Comment 5 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.
Filip Pizlo
Comment 6 2015-08-11 20:14:57 PDT
Created attachment 258798 [details] patch for landing
WebKit Commit Bot
Comment 7 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.
Filip Pizlo
Comment 8 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.
Filip Pizlo
Comment 9 2015-08-11 21:21:08 PDT
Landed in http://trac.webkit.org/changeset/188323
Note You need to log in before you can comment on or make changes to this bug.