Bug 147908

Summary:

Always use a byte-sized lock implementation

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

Web Template Framework

Assignee:

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:

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

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