Create a reader-writer lock implementaiton in base.

Create a reader-writer lock implementaiton in base.

Committed: https://crrev.com/df6b66eec33d69c0d321e22a10740738fccf2dd7
Cr-Commit-Position: refs/heads/master@{#397034}

[Anand Mistry (off Chromium), 2016-05-18 00:26:06]

amistry@chromium.org changed reviewers:
+ danakj@chromium.org

[Anand Mistry (off Chromium), 2016-05-18 00:26:07]

WDYT?

[danakj, 2016-05-19 23:01:37]

Seems useful, and like something we want for MessageLoop based on
https://codereview.chromium.org/1991623002/.

Few comments.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock.h (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock.h:25: class BASE_EXPORT RWLock {
ReadWriteLock or ReaderWriterLock. Then make the file name match.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock.h:64: explicit AutoReadLock(RWLock &lock) :
lock_(lock) {
the & should go on the type not the variable name

This is actually a style guide violation and should be taking a pointer. I know
you're being consistent with AutoLock which violates this also.. so.. I guess..

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock.h:78: explicit AutoWriteLock(RWLock &lock) :
lock_(lock) {
ditto

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock_nacl.cc (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_nacl.cc:18: AutoLock l(native_handle_);
nit: name var "hold". ditto elsewhere

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock_posix.cc (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_posix.cc:11: RWLock::RWLock() {
can you use PTHREAD_RWLOCK_INITIALIZER similar to windows?

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_posix.cc:12: int rv =
pthread_rwlock_init(&native_handle_, nullptr);
nit: name "result" instead of rv. ditto elsewhere

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock_unittest.cc (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:16: // Basic test to make sure that
*Acquire()/*Release() don't crash ----------
replace the " ----------" with "." and ditto elsewhere

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:53: for (int i = 0; i < 5; i++) {
I don't really get the point of this test. What is it testing?

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:83: lock_->ReadAcquire();
Use AutoReadLock

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:146:
WriterRWLockTestThread::DoStuff(&lock, &value);
This seems like you could write a bit more obvious test something like:

- Thread 1 takes a read lock.
- Thread 2 tries to take a write lock and change the value.
- Thread 1 waits a bit of time, checks to see the value hasn't changed.
- Thread 1 releases the read lock, waits for signal from the thread 2.
- Thread 1 sees the value has changed.

wdyt? This looping stuff seems a bit weird to me.

You could then add a thread 3 that also does a read lock and ensure it's able to
grab it before releasing the read lock on thread 1.

[Anand Mistry (off Chromium), 2016-05-20 04:06:39]

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock.h (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock.h:25: class BASE_EXPORT RWLock {
On 2016/05/19 23:01:36, danakj wrote:
> ReadWriteLock or ReaderWriterLock. Then make the file name match.

Done.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock.h:64: explicit AutoReadLock(RWLock &lock) :
lock_(lock) {
On 2016/05/19 23:01:36, danakj wrote:
> the & should go on the type not the variable name
> 
> This is actually a style guide violation and should be taking a pointer. I
know
> you're being consistent with AutoLock which violates this also.. so.. I
guess..

Done.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock.h:78: explicit AutoWriteLock(RWLock &lock) :
lock_(lock) {
On 2016/05/19 23:01:36, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock_nacl.cc (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_nacl.cc:18: AutoLock l(native_handle_);
On 2016/05/19 23:01:37, danakj wrote:
> nit: name var "hold". ditto elsewhere

Done.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock_posix.cc (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_posix.cc:11: RWLock::RWLock() {
On 2016/05/19 23:01:37, danakj wrote:
> can you use PTHREAD_RWLOCK_INITIALIZER similar to windows?

Done.

Oddly enough, the man on Linux doesn't say this exists, but it is in SUSv2.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_posix.cc:12: int rv =
pthread_rwlock_init(&native_handle_, nullptr);
On 2016/05/19 23:01:37, danakj wrote:
> nit: name "result" instead of rv. ditto elsewhere

Done.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
File base/synchronization/rw_lock_unittest.cc (right):

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:16: // Basic test to make sure that
*Acquire()/*Release() don't crash ----------
On 2016/05/19 23:01:37, danakj wrote:
> replace the " ----------" with "." and ditto elsewhere

Done.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:53: for (int i = 0; i < 5; i++) {
On 2016/05/19 23:01:37, danakj wrote:
> I don't really get the point of this test. What is it testing?

It's a basic sanity test to ensure that acquisitions that contend don't do
something completely stupid. Arguably, it's not very useful. But I think it
should still be here.

Also, I copied this all from lock_unittest :P

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:83: lock_->ReadAcquire();
On 2016/05/19 23:01:37, danakj wrote:
> Use AutoReadLock

Done.

https://codereview.chromium.org/1988563002/diff/20001/base/synchronization/rw...
base/synchronization/rw_lock_unittest.cc:146:
WriterRWLockTestThread::DoStuff(&lock, &value);
On 2016/05/19 23:01:37, danakj wrote:
> This seems like you could write a bit more obvious test something like:
> 
> - Thread 1 takes a read lock.
> - Thread 2 tries to take a write lock and change the value.
> - Thread 1 waits a bit of time, checks to see the value hasn't changed.
> - Thread 1 releases the read lock, waits for signal from the thread 2.
> - Thread 1 sees the value has changed.
> 
> wdyt? This looping stuff seems a bit weird to me.

Done.

> You could then add a thread 3 that also does a read lock and ensure it's able
to
> grab it before releasing the read lock on thread 1.

This is actually not valid. Windows doesn't support fairness at all, and posix
only does it when using real-time scheduling policies. Ideally, I'd like to have
a write-preferring implementation to prevent writers from being starved (since
writes should be rare). But that would require a custom implementation and not
rely on windows/posix.

[Anand Mistry (off Chromium), 2016-05-20 05:16:55]

I'm a little bit concerned about people using this in places it's unnecessary
(i.e. there's no demonstrable contention) or incorrect. How do you feel about
adding a warning telling people to use base::Lock instead unless you can show
that using a ReaderWriteLock is useful?

[Anand Mistry (off Chromium), 2016-05-25 23:37:30]

On 2016/05/20 05:16:55, Anand Mistry wrote:
> I'm a little bit concerned about people using this in places it's unnecessary
> (i.e. there's no demonstrable contention) or incorrect. How do you feel about
> adding a warning telling people to use base::Lock instead unless you can show
> that using a ReaderWriteLock is useful?

Friendly ping!

[danakj, 2016-05-25 23:44:06]

On Wed, May 25, 2016 at 4:37 PM, <amistry@chromium.org> wrote:

> On 2016/05/20 05:16:55, Anand Mistry wrote:
> > I'm a little bit concerned about people using this in places it's
> unnecessary
> > (i.e. there's no demonstrable contention) or incorrect. How do you feel
> about
> > adding a warning telling people to use base::Lock instead unless you can
> show
> > that using a ReaderWriteLock is useful?
>
> Friendly ping!
>

Ya, I've been thinking about this. I think put this in base::subtle
namespace with a comment saying to use Lock instead, explaining when you
might want to use this, and a warning that you're not guaranteed progress.


>
> https://codereview.chromium.org/1988563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Anand Mistry (off Chromium), 2016-05-26 01:32:58]

On 2016/05/25 23:44:06, danakj wrote:
> On Wed, May 25, 2016 at 4:37 PM, <mailto:amistry@chromium.org> wrote:
> 
> > On 2016/05/20 05:16:55, Anand Mistry wrote:
> > > I'm a little bit concerned about people using this in places it's
> > unnecessary
> > > (i.e. there's no demonstrable contention) or incorrect. How do you feel
> > about
> > > adding a warning telling people to use base::Lock instead unless you can
> > show
> > > that using a ReaderWriteLock is useful?
> >
> > Friendly ping!
> >
> 
> Ya, I've been thinking about this. I think put this in base::subtle
> namespace with a comment saying to use Lock instead, explaining when you
> might want to use this, and a warning that you're not guaranteed progress.

I've move to base::subtle and added a warning. But I've opted not to give any
advice as to when you might want to use this, following atomicops and just
saying "don't use this". I figured that anyone that reads the warning and
understand their problem well enough to use this, will use this.

> 
> 
> >
> > https://codereview.chromium.org/1988563002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-05-31 22:10:01]

LGTM

[Anand Mistry (off Chromium), 2016-06-01 00:24:08]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-06-01 00:24:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988563002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988563002/100001

[commit-bot: I haz the power, 2016-06-01 02:52:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-01 02:54:05]

Description was changed from

==========
Create a reader-writer lock implementaiton in base.
==========

to

==========
Create a reader-writer lock implementaiton in base.

Committed: https://crrev.com/df6b66eec33d69c0d321e22a10740738fccf2dd7
Cr-Commit-Position: refs/heads/master@{#397034}
==========

[commit-bot: I haz the power, 2016-06-01 02:54:06]

Patchset 6 (id:??) landed as
https://crrev.com/df6b66eec33d69c0d321e22a10740738fccf2dd7
Cr-Commit-Position: refs/heads/master@{#397034}