Switch critical section locks out for atomic operations

Switch critical section locks out for atomic operations

BUG=chromium:581029
Committed: https://crrev.com/533a4e4882ac1b9bf84a4f30af8d4eeaaf903a8d
Cr-Commit-Position: refs/heads/master@{#11413}

[tommi, 2016-01-25 18:03:20]

Fix build error

[tommi, 2016-01-25 18:05:02]

tommi@webrtc.org changed reviewers:
+ peah@webrtc.org

[tommi, 2016-01-25 18:05:03]

Per - can you take a look at this and we can discuss the behavioural difference?
 I'm worried that as a performance measuring test, it might be affecting the
work its measuring.  However, I don't have the full picture.

[peah-webrtc, 2016-01-26 08:48:45]

Hi,
I think that the changes looks good.

Ideally, the LockedFlag class should be replaced by an event, but it should work
as it is.

Regarding the spinlock construction, I don't see why it should not work, but it
should be possible to replace it with an event.

https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_performance_unittest.cc
(right):

https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_performance_unittest.cc:203: //
Class that represents a flag that can only be raised.
This class should be replaced with an rtc:Event as that is exactly what it does.

https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_performance_unittest.cc:683: //
meant to measure.  See if we could use events instead to signal readiness.
Yes, I think an event would be better to use here if there is a problem that one
thread is starving the other in the sense that the threads are not really
running in parallel.

[tommi, 2016-01-28 07:38:40]

On 2016/01/26 08:48:45, peah-webrtc wrote:
> Hi,
> I think that the changes looks good.
> 
> Ideally, the LockedFlag class should be replaced by an event, but it should
work
> as it is.
> 
> Regarding the spinlock construction, I don't see why it should not work, but
it
> should be possible to replace it with an event.
> 
>
https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/audio_processing_performance_unittest.cc
> (right):
> 
>
https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/audio_processing_performance_unittest.cc:203:
//
> Class that represents a flag that can only be raised.
> This class should be replaced with an rtc:Event as that is exactly what it
does.
> 
>
https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/audio_processing_performance_unittest.cc:683:
//
> meant to measure.  See if we could use events instead to signal readiness.
> Yes, I think an event would be better to use here if there is a problem that
one
> thread is starving the other in the sense that the threads are not really
> running in parallel.

I agree with all your comments.  I don't have time unfortunately to address them
just yet, so what do you think about landing the CL as is to see the impact and
I'll address the comments in a follow up cl?

[peah-webrtc, 2016-01-28 08:23:35]

On 2016/01/28 07:38:40, tommi-webrtc wrote:
> On 2016/01/26 08:48:45, peah-webrtc wrote:
> > Hi,
> > I think that the changes looks good.
> > 
> > Ideally, the LockedFlag class should be replaced by an event, but it should
> work
> > as it is.
> > 
> > Regarding the spinlock construction, I don't see why it should not work, but
> it
> > should be possible to replace it with an event.
> > 
> >
>
https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
> > File
webrtc/modules/audio_processing/audio_processing_performance_unittest.cc
> > (right):
> > 
> >
>
https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
> >
webrtc/modules/audio_processing/audio_processing_performance_unittest.cc:203:
> //
> > Class that represents a flag that can only be raised.
> > This class should be replaced with an rtc:Event as that is exactly what it
> does.
> > 
> >
>
https://codereview.webrtc.org/1635563002/diff/20001/webrtc/modules/audio_proc...
> >
webrtc/modules/audio_processing/audio_processing_performance_unittest.cc:683:
> //
> > meant to measure.  See if we could use events instead to signal readiness.
> > Yes, I think an event would be better to use here if there is a problem that
> one
> > thread is starving the other in the sense that the threads are not really
> > running in parallel.
> 
> I agree with all your comments.  I don't have time unfortunately to address
them
> just yet, so what do you think about landing the CL as is to see the impact
and
> I'll address the comments in a follow up cl?

That sounds great!

lgtm

[tommi, 2016-01-28 09:43:42]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2016-01-28 09:43:50]

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

[commit-bot: I haz the power, 2016-01-28 10:44:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-28 10:44:24]

Description was changed from

==========
Switch critical section locks out for atomic operations

BUG=chromium:581029
==========

to

==========
Switch critical section locks out for atomic operations

BUG=chromium:581029

Committed: https://crrev.com/533a4e4882ac1b9bf84a4f30af8d4eeaaf903a8d
Cr-Commit-Position: refs/heads/master@{#11413}
==========

[commit-bot: I haz the power, 2016-01-28 10:44:27]

Patchset 2 (id:??) landed as
https://crrev.com/533a4e4882ac1b9bf84a4f30af8d4eeaaf903a8d
Cr-Commit-Position: refs/heads/master@{#11413}