Add countdown_latch

[Alasdair, 2009-11-30 23:06:43]

Please ignore concurrent_priority_queue_test.cc - I'm playing with it to add
some concurrent tests. (Is there a way to tell upload.py to ignore some files
that are being edited?)

[Alasdair, 2009-12-01 20:03:10]

Updated version of this CL, using Mercurial patches to limit it to the correct
set of files. Should now be ready for review...

[Jeffrey Yasskin, 2009-12-02 07:39:41]

http://codereview.appspot.com/164050/diff/27/1023
File include/countdown_latch.h (right):

http://codereview.appspot.com/164050/diff/27/1023#newcode6
include/countdown_latch.h:6: #include <stdlib.h>
What are you using from stdlib.h?

http://codereview.appspot.com/164050/diff/27/1023#newcode11
include/countdown_latch.h:11: // TODO(alasdair): Remove this once
multiple-thread issue with
It's been fixed, right?

http://codereview.appspot.com/164050/diff/27/1023#newcode18
include/countdown_latch.h:18: // to wait() will block until the count reaches
zero.
We should also guarantee that if there's a unique waiter, it's legal for that
waiter to destroy the countdown_latch as soon as wait returns. With an atomic
implementation, that's not as trivial to guarantee as you'd expect (you do it
with an empty critical section in the destructor), but it's really useful.

http://codereview.appspot.com/164050/diff/27/1023#newcode19
include/countdown_latch.h:19: class countdown_latch {
wait() and count_down() should document the happens-before edges they guarantee.
For example, "all calls to count_down() happen before any call to wait()
returns."

http://codereview.appspot.com/164050/diff/27/1023#newcode26
include/countdown_latch.h:26: // threads are blocked in wait(), the behaviour is
undefined.
Or while other threads may be calling count_down, right?

http://codereview.appspot.com/164050/diff/27/1023#newcode35
include/countdown_latch.h:35: // no-op. Returns the updated value of count.
I suspect that this should be undefined if the count is already 0.

The comment should mention that each caller will receive a unique value. I'm not
sure that's useful though, and could reduce the concurrency. Do you have a use
case in mind?

The internal version returns true for the last call, which can let people avoid
having a waiter thread.

http://codereview.appspot.com/164050/diff/27/1023#newcode38
include/countdown_latch.h:38: // Gets the current value of the count
This is a semi-dangerous operation since the value can change by the time the
caller does anything with it. I don't think we should include it unless we have
a use case in mind.

http://codereview.appspot.com/164050/diff/27/1023#newcode49
include/countdown_latch.h:49: mutex count_mutex_;
There's no need to have multiple mutexes in this class.

http://codereview.appspot.com/164050/diff/27/1024
File src/countdown_latch.cc (right):

http://codereview.appspot.com/164050/diff/27/1024#newcode6
src/countdown_latch.cc:6: #include <pthread.h>
I don't think you're using pthread.h anymore.

http://codereview.appspot.com/164050/diff/27/1024#newcode24
src/countdown_latch.cc:24: int count = decrement_count();
decrement_count() seems unnecessary. How about

unsigned int countdown_latch::count_down() {
  lock_guard<mutex> l(mu_);
  --count_;
  if (count_ == 0) {
    condition_.notify_all();
  }
  return count_;
}

With atomics, it would look something like:

unsigned int countdown_latch::count_down() {
  unsigned new_count = count_.fetch_sub(1, memory_order_acq_rel) - 1;
  if (new_count == 0) {
    lock_guard<mutex> l(mu_);  // Synchronize with destructor.
    condition_.notify_all();
  }
  return new_count;
}

http://codereview.appspot.com/164050/diff/27/1025
File testing/countdown_latch_test.cc (right):

http://codereview.appspot.com/164050/diff/27/1025#newcode14
testing/countdown_latch_test.cc:14: class CountdownLatchTest : public
testing::Test {
Please also test that wait() can guard destruction of the latch.

You'll need to put the threaded tests in a loop so that tsan has a better chance
of seeing the races happening.

http://codereview.appspot.com/164050/diff/27/1025#newcode38
testing/countdown_latch_test.cc:38: // Tests two threads waiting on a single
countdown_latch
You should assert something. Write code that would have a race if the latch
didn't work.

[Alasdair, 2009-12-02 18:22:52]

I've left a couple of items as TODOS which I will address before submitting, but
I wanted to get the simpler changes fixed immediately.

http://codereview.appspot.com/164050/diff/27/1023
File include/countdown_latch.h (right):

http://codereview.appspot.com/164050/diff/27/1023#newcode6
include/countdown_latch.h:6: #include <stdlib.h>
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> What are you using from stdlib.h?

I was using some stuff, but it's gone now, Well spotted. I've removed this.

http://codereview.appspot.com/164050/diff/27/1023#newcode11
include/countdown_latch.h:11: // TODO(alasdair): Remove this once
multiple-thread issue with
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> It's been fixed, right?

Oops, yes it has

http://codereview.appspot.com/164050/diff/27/1023#newcode18
include/countdown_latch.h:18: // to wait() will block until the count reaches
zero.
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> We should also guarantee that if there's a unique waiter, it's legal for that
> waiter to destroy the countdown_latch as soon as wait returns. With an atomic
> implementation, that's not as trivial to guarantee as you'd expect (you do it
> with an empty critical section in the destructor), but it's really useful.

OK, this sounds like a useful thing. I'll look at adding it.

http://codereview.appspot.com/164050/diff/27/1023#newcode19
include/countdown_latch.h:19: class countdown_latch {
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> wait() and count_down() should document the happens-before edges they
guarantee.
> For example, "all calls to count_down() happen before any call to wait()
> returns."

Done.

http://codereview.appspot.com/164050/diff/27/1023#newcode26
include/countdown_latch.h:26: // threads are blocked in wait(), the behaviour is
undefined.
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> Or while other threads may be calling count_down, right?

Done.

http://codereview.appspot.com/164050/diff/27/1023#newcode35
include/countdown_latch.h:35: // no-op. Returns the updated value of count.
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> I suspect that this should be undefined if the count is already 0.

I was basing this on the Java CountDownLatch() where decrementing beyond 0 is a
no-op. We can change it if you think there's a reason, but I was assuming that
following the Java API would be sensible if there was no compelling reason to do
otherwise. I suppose you could argue that decrementing beyond 0 is an error of
some kind, and we might as well raise it straight away. I'm easy on this, so let
me know what you think.

> The comment should mention that each caller will receive a unique value. I'm
not
> sure that's useful though, and could reduce the concurrency. Do you have a use
> case in mind?

I don't quite see why it would reduce concurrency? We're returning a local
stack-based variable (see countdown_latch.cc) which should be unique per thread?

That said, I've just violated my follow-the Java-api rule, so I can make this a
void if you prefer. Just let me know.

> The internal version returns true for the last call, which can let people
avoid
> having a waiter thread.

http://codereview.appspot.com/164050/diff/27/1023#newcode38
include/countdown_latch.h:38: // Gets the current value of the count
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> This is a semi-dangerous operation since the value can change by the time the
> caller does anything with it. I don't think we should include it unless we
have
> a use case in mind.

Again, I was partly basing this on the Java version. I can remove it if you
think we don't need it, although I think it's useful for testing.

http://codereview.appspot.com/164050/diff/27/1023#newcode49
include/countdown_latch.h:49: mutex count_mutex_;
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> There's no need to have multiple mutexes in this class.

Until we have atomics, I think this is necessary for implementing
decrement_count(). Am I missing something?

http://codereview.appspot.com/164050/diff/27/1024
File src/countdown_latch.cc (right):

http://codereview.appspot.com/164050/diff/27/1024#newcode6
src/countdown_latch.cc:6: #include <pthread.h>
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> I don't think you're using pthread.h anymore.

Nor stdlib. Removed.

http://codereview.appspot.com/164050/diff/27/1024#newcode24
src/countdown_latch.cc:24: int count = decrement_count();
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> decrement_count() seems unnecessary. How about
> 
> unsigned int countdown_latch::count_down() {
>   lock_guard<mutex> l(mu_);
>   --count_;
>   if (count_ == 0) {
>     condition_.notify_all();
>   }
>   return count_;
> }
> 
> With atomics, it would look something like:
> 
> unsigned int countdown_latch::count_down() {
>   unsigned new_count = count_.fetch_sub(1, memory_order_acq_rel) - 1;
>   if (new_count == 0) {
>     lock_guard<mutex> l(mu_);  // Synchronize with destructor.
>     condition_.notify_all();
>   }
>   return new_count;
> }

I did it like this so that when we get atomics I could just update
decrement_count and leave this method untouched. I don't really mind either way.
If you prefer to get rid of decrement_count I'll do that.

http://codereview.appspot.com/164050/diff/27/1025
File testing/countdown_latch_test.cc (right):

http://codereview.appspot.com/164050/diff/27/1025#newcode14
testing/countdown_latch_test.cc:14: class CountdownLatchTest : public
testing::Test {
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> Please also test that wait() can guard destruction of the latch.

I'll add that.

> You'll need to put the threaded tests in a loop so that tsan has a better
chance
> of seeing the races happening.

Done. Is 100 loops enough?

http://codereview.appspot.com/164050/diff/27/1025#newcode38
testing/countdown_latch_test.cc:38: // Tests two threads waiting on a single
countdown_latch
On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> You should assert something. Write code that would have a race if the latch
> didn't work.

I've added something that tries to test for this. Unfortunately there's no way
as far as I know to verify that a thread is blocked, so that test as written
could still fail if the thread scheduling was screwy. (It does actually fail if
I make wait a no-op, but it's still not guaranteed.)

Thoughts?

[Jeffrey Yasskin, 2009-12-03 03:14:28]

http://codereview.appspot.com/164050/diff/27/1023
File include/countdown_latch.h (right):

http://codereview.appspot.com/164050/diff/27/1023#newcode18
include/countdown_latch.h:18: // to wait() will block until the count reaches
zero.
On 2009/12/02 18:22:52, Alasdair wrote:
> On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > We should also guarantee that if there's a unique waiter, it's legal for
that
> > waiter to destroy the countdown_latch as soon as wait returns. With an
atomic
> > implementation, that's not as trivial to guarantee as you'd expect (you do
it
> > with an empty critical section in the destructor), but it's really useful.
> 
> OK, this sounds like a useful thing. I'll look at adding it.

It's already guaranteed by your current implementation. You just need to
document it. The atomic-ified implementation has to jump through a hoop to keep
supporting it, but you haven't gone there yet.

http://codereview.appspot.com/164050/diff/27/1023#newcode35
include/countdown_latch.h:35: // no-op. Returns the updated value of count.
On 2009/12/02 18:22:52, Alasdair wrote:
> On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > I suspect that this should be undefined if the count is already 0.
> 
> I was basing this on the Java CountDownLatch() where decrementing beyond 0 is
a
> no-op. We can change it if you think there's a reason, but I was assuming that
> following the Java API would be sensible if there was no compelling reason to
do
> otherwise. I suppose you could argue that decrementing beyond 0 is an error of
> some kind, and we might as well raise it straight away. I'm easy on this, so
let
> me know what you think.

"Follow Java" is a very persuasive argument. :)
Let's see: If anyone decrements beyond 0, then a single waiter no longer has the
right to delete the object.  The fact that Java doesn't have to worry about
destruction may justify having a different API, but on the other hand, we can
just document that extra restriction. On the third hand, accidentally having too
many workers could cause occasional crashes, which would be hard to debug, and
having an intentional crash at the extra count_down() would be nice.

> > The comment should mention that each caller will receive a unique value. I'm
> not
> > sure that's useful though, and could reduce the concurrency. Do you have a
use
> > case in mind?
> 
> I don't quite see why it would reduce concurrency? We're returning a local
> stack-based variable (see countdown_latch.cc) which should be unique per
thread?

Giving each caller a unique value requires having a single integer that they all
decrement atomically, which causes cache contention. Avoiding that requirement
would allow us to, for example, shard the count by CPU, decrement the CPU-local
count most of the time, and only examine other CPUs' counters when the CPU-local
one hit 0. (I haven't worked out the algorithm for this, so it may not be
possible, but it's the kind of thing you can do if you don't grant your users
any global knowledge.)

The fact that the shared counter gets copied to the stack before being returned
doesn't help the fact that shared counters can be expensive.

> That said, I've just violated my follow-the Java-api rule, so I can make this
a
> void if you prefer. Just let me know.

I would prefer for now.

> > The internal version returns true for the last call, which can let people
> avoid
> > having a waiter thread.

http://codereview.appspot.com/164050/diff/27/1023#newcode38
include/countdown_latch.h:38: // Gets the current value of the count
On 2009/12/02 18:22:52, Alasdair wrote:
> On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > This is a semi-dangerous operation since the value can change by the time
the
> > caller does anything with it. I don't think we should include it unless we
> have
> > a use case in mind.
> 
> Again, I was partly basing this on the Java version. I can remove it if you
> think we don't need it, although I think it's useful for testing. 
> 

I at least think it should have a warning saying what's guaranteed and what's
not in the presence of concurrent calls to count_down.

http://codereview.appspot.com/164050/diff/27/1023#newcode49
include/countdown_latch.h:49: mutex count_mutex_;
On 2009/12/02 18:22:52, Alasdair wrote:
> On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > There's no need to have multiple mutexes in this class.
> 
> Until we have atomics, I think this is necessary for implementing
> decrement_count(). Am I missing something?

It's almost always legal to combine two mutexes. The two risks I can think of
are if you then need the combined mutex to be recursive, and if contention
increases. Here, I don't think either applies.  Could you describe the bad
behavior you think results if there's only one mutex?

http://codereview.appspot.com/164050/diff/27/1024
File src/countdown_latch.cc (right):

http://codereview.appspot.com/164050/diff/27/1024#newcode24
src/countdown_latch.cc:24: int count = decrement_count();
On 2009/12/02 18:22:52, Alasdair wrote:
> On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > decrement_count() seems unnecessary. How about
> > 
> > unsigned int countdown_latch::count_down() {
> >   lock_guard<mutex> l(mu_);
> >   --count_;
> >   if (count_ == 0) {
> >     condition_.notify_all();
> >   }
> >   return count_;
> > }
> > 
> > With atomics, it would look something like:
> > 
> > unsigned int countdown_latch::count_down() {
> >   unsigned new_count = count_.fetch_sub(1, memory_order_acq_rel) - 1;
> >   if (new_count == 0) {
> >     lock_guard<mutex> l(mu_);  // Synchronize with destructor.
> >     condition_.notify_all();
> >   }
> >   return new_count;
> > }
> 
> I did it like this so that when we get atomics I could just update
> decrement_count and leave this method untouched. I don't really mind either
way.
> If you prefer to get rid of decrement_count I'll do that.
> 

I think we'll want to merge the methods when Lawrence checks in the atomics
anyway, since the merged version is shorter.

The 1-critical-section version also makes it easier to show that wait() allows
destruction, although I think your current code in wait() makes it legal too.

So I'd prefer removing decrement_count.

http://codereview.appspot.com/164050/diff/27/1025
File testing/countdown_latch_test.cc (right):

http://codereview.appspot.com/164050/diff/27/1025#newcode14
testing/countdown_latch_test.cc:14: class CountdownLatchTest : public
testing::Test {
On 2009/12/02 18:22:52, Alasdair wrote:
> On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > Please also test that wait() can guard destruction of the latch.
> 
> I'll add that.
> 
> > You'll need to put the threaded tests in a loop so that tsan has a better
> chance
> > of seeing the races happening.
> 
> Done. Is 100 loops enough?

Should be, thanks.

http://codereview.appspot.com/164050/diff/39/43#newcode17
testing/countdown_latch_test.cc:17: void WaitForLatch(countdown_latch& latch) {
I'm skeptical of using the whole std namespace. Just use the things you need out
of it.

http://codereview.appspot.com/164050/diff/39/43#newcode41
testing/countdown_latch_test.cc:41: thread t1(tr1::bind(WaitForLatch,
tr1::ref(latch)));
You can probably copy some tests for this out of the internal tests for our
analogous class.

http://codereview.appspot.com/164050/diff/39/43#newcode49
testing/countdown_latch_test.cc:49: // Tests two threads waiting on a
countdown_latch that has already
The lock inside this counter is likely to throw off detection of any races that
happen here. You need to read the state immediately after wait() returns, and
only then run through any code that might lock.

http://codereview.appspot.com/164050/diff/39/43#newcode50
testing/countdown_latch_test.cc:50: // been decremented.
Unfortunately, EXPECT_EQ isn't thread-safe. What you can do is write to an
out-parameter and EXPECT on it from the main thread. Use thread.join() or
another countdown_latch to guarantee that the out parameter has been written
before reading it.

http://codereview.appspot.com/164050/diff/39/43#newcode75
testing/countdown_latch_test.cc:75: 
Please name your tests according to what behavior they're testing, not just what
they use to test it.

http://codereview.appspot.com/164050/diff/39/43#newcode86
testing/countdown_latch_test.cc:86: 
Oops, just "10" here shouldn't compile. I must have forgotten an "explicit" on
the milliseconds constructor. :(

http://codereview.appspot.com/164050/diff/39/43#newcode87
testing/countdown_latch_test.cc:87: 
Ew @ sleeping. Either use another countdown latch to wait for the thread to
start, or write the waiting logic explicitly with a condition variable.

http://codereview.appspot.com/164050/diff/39/43#newcode94
testing/countdown_latch_test.cc:94: 
This verifies that they've gotten to the wait() point, not that they haven't
passed it. You'd need to EXPECT_EQ(0, count1.get_count()) for that.

You only need 1 other thread to assert that wait() actually blocks. 2 threads is
just noise.

http://codereview.appspot.com/164050/diff/39/43#newcode108
testing/countdown_latch_test.cc:108: 
No reason to count down twice here.

You also don't need other threads for this test. You can just write:

countdown_latch latch(1);
latch.count_down();
latch.wait();
// If test finishes, it passes.

http://codereview.appspot.com/164050/diff/39/43#newcode117
testing/countdown_latch_test.cc:117: 
You should comment more precisely what bad behavior would make this test fail.
If you're just randomly using the latches, the test probably isn't worth the
time it takes.

[Alasdair, 2009-12-08 02:23:25]

http://codereview.appspot.com/164050/diff/27/1023
File include/countdown_latch.h (right):

http://codereview.appspot.com/164050/diff/27/1023#newcode18
include/countdown_latch.h:18: // to wait() will block until the count reaches
zero.

I may be misunderstanding pthread condition variables. 

When we call notify_all() inside count_down(), does the waiting thread wake up
immediately, or does it only wake up when we release the condition_mutex_?

I had thought that it would wake straight away. If it did, then the
countdown_latch could get destroyed before we return from count_down(). But if
the waiting thread only wakes up when we release the condition_mutex then yes,
the current implementaion is safe.

http://codereview.appspot.com/164050/diff/27/1023#newcode35
include/countdown_latch.h:35: // no-op. Returns the updated value of count.
On 2009/12/03 03:14:28, Jeffrey Yasskin wrote:
> On 2009/12/02 18:22:52, Alasdair wrote:
> > On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > > I suspect that this should be undefined if the count is already 0.
> > 
> > I was basing this on the Java CountDownLatch() where decrementing beyond 0
is
> a
> > no-op. We can change it if you think there's a reason, but I was assuming
that
> > following the Java API would be sensible if there was no compelling reason
to
> do
> > otherwise. I suppose you could argue that decrementing beyond 0 is an error
of
> > some kind, and we might as well raise it straight away. I'm easy on this, so
> let
> > me know what you think.
> 
> "Follow Java" is a very persuasive argument. :)
> Let's see: If anyone decrements beyond 0, then a single waiter no longer has
the
> right to delete the object.  The fact that Java doesn't have to worry about
> destruction may justify having a different API, but on the other hand, we can
> just document that extra restriction. On the third hand, accidentally having
too
> many workers could cause occasional crashes, which would be hard to debug, and
> having an intentional crash at the extra count_down() would be nice.

OK, I'll buy that. I'll make a decrement beyond 0 an exception. 


> Giving each caller a unique value requires having a single integer that they
all
> decrement atomically, which causes cache contention. Avoiding that requirement
> would allow us to, for example, shard the count by CPU, decrement the
CPU-local
> count most of the time, and only examine other CPUs' counters when the
CPU-local
> one hit 0. (I haven't worked out the algorithm for this, so it may not be
> possible, but it's the kind of thing you can do if you don't grant your users
> any global knowledge.)

Again. I'll buy that. I'll make it void.

http://codereview.appspot.com/164050/diff/27/1023#newcode38
include/countdown_latch.h:38: // Gets the current value of the count
On 2009/12/03 03:14:28, Jeffrey Yasskin wrote:
> On 2009/12/02 18:22:52, Alasdair wrote:
> > On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > > This is a semi-dangerous operation since the value can change by the time
> the
> > > caller does anything with it. I don't think we should include it unless we
> > have
> > > a use case in mind.
> > 
> > Again, I was partly basing this on the Java version. I can remove it if you
> > think we don't need it, although I think it's useful for testing. 
> > 
> 
> I at least think it should have a warning saying what's guaranteed and what's
> not in the presence of concurrent calls to count_down.

If we're removing the return from count_down to cover the potential multi-cpu
case then I think we can nix this method too.

http://codereview.appspot.com/164050/diff/27/1023#newcode49
include/countdown_latch.h:49: mutex count_mutex_;
On 2009/12/03 03:14:28, Jeffrey Yasskin wrote:
> On 2009/12/02 18:22:52, Alasdair wrote:
> > On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > > There's no need to have multiple mutexes in this class.
> > 
> > Until we have atomics, I think this is necessary for implementing
> > decrement_count(). Am I missing something?
> 
> It's almost always legal to combine two mutexes. The two risks I can think of
> are if you then need the combined mutex to be recursive, and if contention
> increases. Here, I don't think either applies.  Could you describe the bad
> behavior you think results if there's only one mutex?

If one thread called get_count while another was calling decrement_count() then
without a mutex (and without an atomic count) you could read an invalid value. I
didn't want to use the general wait() mutex here, so I added this one. However,
if get_count() is going away, then this can go too.

http://codereview.appspot.com/164050/diff/27/1024
File src/countdown_latch.cc (right):

http://codereview.appspot.com/164050/diff/27/1024#newcode24
src/countdown_latch.cc:24: int count = decrement_count();
On 2009/12/03 03:14:28, Jeffrey Yasskin wrote:
> On 2009/12/02 18:22:52, Alasdair wrote:
> > On 2009/12/02 07:39:41, Jeffrey Yasskin wrote:
> > > decrement_count() seems unnecessary. How about
> > > 
> > > unsigned int countdown_latch::count_down() {
> > >   lock_guard<mutex> l(mu_);
> > >   --count_;
> > >   if (count_ == 0) {
> > >     condition_.notify_all();
> > >   }
> > >   return count_;
> > > }
> > > 
> > > With atomics, it would look something like:
> > > 
> > > unsigned int countdown_latch::count_down() {
> > >   unsigned new_count = count_.fetch_sub(1, memory_order_acq_rel) - 1;
> > >   if (new_count == 0) {
> > >     lock_guard<mutex> l(mu_);  // Synchronize with destructor.
> > >     condition_.notify_all();
> > >   }
> > >   return new_count;
> > > }
> > 
> > I did it like this so that when we get atomics I could just update
> > decrement_count and leave this method untouched. I don't really mind either
> way.
> > If you prefer to get rid of decrement_count I'll do that.
> > 
> 
> I think we'll want to merge the methods when Lawrence checks in the atomics
> anyway, since the merged version is shorter.
> 
> The 1-critical-section version also makes it easier to show that wait() allows
> destruction, although I think your current code in wait() makes it legal too.
> 
> So I'd prefer removing decrement_count.

OK, will do.

http://codereview.appspot.com/164050/diff/39/43
File testing/countdown_latch_test.cc (right):

http://codereview.appspot.com/164050/diff/39/43#newcode1
testing/countdown_latch_test.cc:1: // Copyright 2009 Google Inc. All Rights
Reserved.
I'd like to rethink how this test works. The removal of get_count() means that
we need a better way of determining that the threads actually blocked as
expected. I had some ideas for implementing a more generic mechanism for
monitoring thread state, and I wanted to take a look at that before writing the
test.

Are you OK with my submitting the implementation and leaving the test for a
subsequent CL?

[Alasdair, 2009-12-18 21:42:59]

Merge with new Makefile

[Jeffrey Yasskin, 2009-12-18 22:06:21]

LGTM with just a couple nits. Please commit.

http://codereview.appspot.com/164050/diff/6001/6004
File include/countdown_latch.h (right):

http://codereview.appspot.com/164050/diff/6001/6004#newcode21
include/countdown_latch.h:21: // other threads are blocked in wait(), or ar
invoking count_down(),
sp: ar

http://codereview.appspot.com/164050/diff/6001/6004#newcode25
include/countdown_latch.h:25: // TODO(alasdair): before submit, add guard to
allow this to be
"before submit" todos should be fixed or decided against before submitting.

I think this one's already done.

http://codereview.appspot.com/164050/diff/6001/6004#newcode35
include/countdown_latch.h:35: // throws std::system_error
+ "."

[Alasdair, 2010-03-19 20:59:12]

Just cleaning up old issues. I never published my final response, but all
remaining issues were cleaned up and the changes committed.

http://codereview.appspot.com/164050/diff/6001/6004
File include/countdown_latch.h (right):

http://codereview.appspot.com/164050/diff/6001/6004#newcode21
include/countdown_latch.h:21: // other threads are blocked in wait(), or ar
invoking count_down(),
On 2009/12/18 22:06:21, Jeffrey Yasskin wrote:
> sp: ar

Done.

http://codereview.appspot.com/164050/diff/6001/6004#newcode25
include/countdown_latch.h:25: // TODO(alasdair): before submit, add guard to
allow this to be
On 2009/12/18 22:06:21, Jeffrey Yasskin wrote:
> "before submit" todos should be fixed or decided against before submitting.
> 
> I think this one's already done.

Done.

http://codereview.appspot.com/164050/diff/6001/6004#newcode35
include/countdown_latch.h:35: // throws std::system_error
On 2009/12/18 22:06:21, Jeffrey Yasskin wrote:
> + "."

Done.