Signal condition variable in the critical section.

Signal condition variable in the critical section.

Condition variables should be signaled inside the critical section.
cf) http://www.chromium.org/developers/lock-and-condition-variable#inside

BUG=None
TEST=Ran trybots.

Committed: https://crrev.com/66a8b6a9986f9e918edf5d1228435b8e76a40c8d
Cr-Commit-Position: refs/heads/master@{#293471}

[hidehiko, 2014-09-03 12:30:38]

hidehiko@chromium.org changed reviewers:
+ mseaborn@chromium.org

[hidehiko, 2014-09-03 12:30:39]

Thank you for review in advance,
- hidehiko

[Junichi Uekawa, 2014-09-04 02:17:34]

On 2014/09/03 12:30:39, hidehiko wrote:
> Thank you for review in advance,
> - hidehiko

That page has lots of "TODO(verify this)", really?

[hamaji, 2014-09-04 09:39:43]

hamaji@chromium.org changed reviewers:
+ hamaji@chromium.org

[hamaji, 2014-09-04 09:39:43]

lgtm

[Mark Seaborn, 2014-09-04 21:48:42]

mseaborn@chromium.org changed reviewers:
+ dmichael@chromium.org, teravest@chromium.org

[Mark Seaborn, 2014-09-04 21:52:23]

On 3 September 2014 05:30, <hidehiko@chromium.org> wrote:

> Reviewers: Mark Seaborn,
>
> Message:
> Thank you for review in advance,
> - hidehiko
>
> Description:
> Signal condition variable in the critical section.
>
> Condition variables should be signaled inside the critical section.
> cf) http://www.chromium.org/developers/lock-and-condition-variable#inside
>

Hmm, I've not read this page before.  It says:

"Why put Signal() inside the critical section?

In most cases, it is correct to put Signal() after the critical section,
but in Chrome code it is always both safe and efficient to put it inside
the critical section. (TODO: verify this)

Some texts recommend putting Signal() after the critical section because
this makes it more likely that the mutex is free when a thread attempts to
reacquire it on return from Wait(). If the Signal() were inside the
critical section, a naive implementation might wake the thread which could
then block once more on the mutex held by the very thread that woke it.

Chrome's condition variables (and most reasonable implementations) detect
this case, and delay waking the waiting thread until the mutex is free.
(TODO: verify this) Hence, there is no performance penalty for calling
Signal() inside the critical section."


I think the claims that this doesn't affect performance are rather dubious,
especially with all of the TODOs.

Roland McGrath and I had a look at glibc's implementation of
pthread_cond_signal().  It doesn't look like it tries to optimise this
case.  So it's not the case that most reasonable implementations detect
this case.  Android and NaCl-newlib's implementations definitely don't
either.  (I implemented NaCl's nc_condvar.c, which is based on Android's
implementation.)

However, LGTM -- there's probably no performance difference in practice.

CC'ing dmichael and teravest because they are probably more familiar with
this part of the code than me.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-09-04 22:24:38]

lgtm

[hidehiko, 2014-09-05 04:21:25]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-09-05 04:22:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/533823003/1

[commit-bot: I haz the power, 2014-09-05 08:15:54]

Committed patchset #1 (id:1) as daa786f988e39992e92d5e31033d0682216a7ac2

[commit-bot: I haz the power, 2014-09-10 03:37:50]

Patchset 1 (id:??) landed as
https://crrev.com/66a8b6a9986f9e918edf5d1228435b8e76a40c8d
Cr-Commit-Position: refs/heads/master@{#293471}