Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1490113004:
Replaced the custom thread synchronization with rtc::Event (Closed)
|
Created:
5 years ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner.
To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone.
However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop.
I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code.
BUG=
Committed: https://crrev.com/631e1345512c592eaebe19d9adeade6ee778624c
Cr-Commit-Position: refs/heads/master@{#10880}
Patch Set 1 #Patch Set 2 : Added proper test termination #
Total comments: 14
Patch Set 3 : Changed event pointers to be passed during construction, added a new framecounter difference functi… #
Total comments: 16
Patch Set 4 : Added const specifiers and refactored the infinite waits in the threads #
Total comments: 7
Messages
Total messages: 25 (9 generated)
Description was changed from ========== Replaced the custom thread synchronization with rtc::Event BUG= ========== to ========== Rewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner. To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone. However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop. I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code. Please let me know what you think! BUG= ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, pbos@webrtc.org
Description was changed from ========== Rewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner. To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone. However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop. I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code. Please let me know what you think! BUG= ========== to ========== Rewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner. To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone. However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop. I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code. BUG= ==========
https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:380: rtc::Event& get_render_call_event() { return render_call_event_; } remove get_ prefix https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:380: rtc::Event& get_render_call_event() { return render_call_event_; } no non-const references allowed (googlestyle), use ptr https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:482: : test_complete_(rtc::Event(false, false)), remove rtc::Event here and below (no copy-constructor) https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:603: TestConfig* test_config, supply events on construction instead of having the public getter in test_, so have a rtc::Event* const render_call_event_; https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:880: -frame_counters_->CaptureMinusRenderCounters() > kMaxCallDifference) { document the unary minus, or a function that's renderminuscapture https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:893: // Set api call flag. put in comment what this should do/trigger https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1089: ASSERT_EQ(true, RunTest()); ASSERT_TRUE(RunTest())
lgtm with pbos's comments.
Thanks for the comments! I have added the proposed changes. On top of that, I also made the critical section in the FrameCounters class mutable so that I could make some of the functions const. https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:380: rtc::Event& get_render_call_event() { return render_call_event_; } On 2015/12/02 13:01:12, pbos-webrtc wrote: > no non-const references allowed (googlestyle), use ptr Thanks! I revised this according to your feedback and now pass the pointers in the constructor instead of calling the accessor method. Acknowledged. https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:380: rtc::Event& get_render_call_event() { return render_call_event_; } On 2015/12/02 13:01:12, pbos-webrtc wrote: > remove get_ prefix Acknowledged. https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:482: : test_complete_(rtc::Event(false, false)), On 2015/12/02 13:01:12, pbos-webrtc wrote: > remove rtc::Event here and below (no copy-constructor) Done. https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:603: TestConfig* test_config, On 2015/12/02 13:01:12, pbos-webrtc wrote: > supply events on construction instead of having the public getter in test_, so > have a rtc::Event* const render_call_event_; Done. https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:880: -frame_counters_->CaptureMinusRenderCounters() > kMaxCallDifference) { On 2015/12/02 13:01:12, pbos-webrtc wrote: > document the unary minus, or a function that's renderminuscapture Good point! I instead created a function for that. Done. https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:893: // Set api call flag. On 2015/12/02 13:01:12, pbos-webrtc wrote: > put in comment what this should do/trigger Done. https://codereview.webrtc.org/1490113004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:1089: ASSERT_EQ(true, RunTest()); On 2015/12/02 13:01:12, pbos-webrtc wrote: > ASSERT_TRUE(RunTest()) Done.
PTAL https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:330: rtc::Event* render_call_event_; * const https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:331: rtc::Event* capture_call_event_; * const https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:376: rtc::Event* render_call_event_; * const https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:377: rtc::Event* capture_call_event_; * const https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:639: render_call_event_->Wait(1); This should wait indefinitely right? Any shutdown will set these events so we should be able to shut down? See implementation suggestion below, WDYT? https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:902: while (!test_->MaybeEndTest() && shouldn't this be: while (true) { if (test_->EndTest()) return false; if (frame_counters_->RenderMinusCaptureCounters() > kMaxCallDifference) capture_call_event_->Wait(rtc::Event::kForever); } And corresponding above. You don't want to run the render side after the test ends right? https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:904: capture_call_event_->Wait(1); Same here, wait indefinitely? https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:916: // Flag to the capture thread that another render api call as occurred has occurred, render API
Thanks for the comments and feedback! -I added the const specifiers. -I also refactored the waiting in the thread functions which made it much better! https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:330: rtc::Event* render_call_event_; On 2015/12/02 15:33:13, pbos-webrtc wrote: > * const Added some more const specifiers as well. Done. https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:331: rtc::Event* capture_call_event_; On 2015/12/02 15:33:13, pbos-webrtc wrote: > * const Added some more const specifiers as well. Done. https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:376: rtc::Event* render_call_event_; On 2015/12/02 15:33:13, pbos-webrtc wrote: > * const Added some more const specifiers as well. Done. https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:377: rtc::Event* capture_call_event_; On 2015/12/02 15:33:13, pbos-webrtc wrote: > * const Added some more const specifiers as well. Done. https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:639: render_call_event_->Wait(1); On 2015/12/02 15:33:13, pbos-webrtc wrote: > This should wait indefinitely right? Any shutdown will set these events so we > should be able to shut down? > > See implementation suggestion below, WDYT? Awesome! Great! Made the change. Done. https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:902: while (!test_->MaybeEndTest() && On 2015/12/02 15:33:13, pbos-webrtc wrote: > shouldn't this be: > > while (true) { > if (test_->EndTest()) > return false; > if (frame_counters_->RenderMinusCaptureCounters() > kMaxCallDifference) > capture_call_event_->Wait(rtc::Event::kForever); > } > > And corresponding above. You don't want to run the render side after the test > ends right? I think you are right. That is the correct way to do it. Done. https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:904: capture_call_event_->Wait(1); On 2015/12/02 15:33:13, pbos-webrtc wrote: > Same here, wait indefinitely? Done. https://codereview.webrtc.org/1490113004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:916: // Flag to the capture thread that another render api call as occurred On 2015/12/02 15:33:13, pbos-webrtc wrote: > has occurred, render API Done.
lgtm with a question and nit https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:444: mutable RandomGenerator rand_gen_; This one looks shared between multiple instances, is it thread safe? https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:903: if (test_->MaybeEndTest()) { Maybe rename MaybeEndTest to TestHasEnded?
On 2015/12/02 22:59:13, pbos-webrtc wrote: > lgtm with a question and nit > > https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc > (right): > > https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:444: > mutable RandomGenerator rand_gen_; > This one looks shared between multiple instances, is it thread safe? > > https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:903: > if (test_->MaybeEndTest()) { > Maybe rename MaybeEndTest to TestHasEnded? The RandomGenerator mutable might be a separate issue, not sure if this is a regression. Thanks for the follow-ups I like this a bunch better. :)
On 2015/12/02 22:59:49, pbos-webrtc wrote: > On 2015/12/02 22:59:13, pbos-webrtc wrote: > > lgtm with a question and nit > > > > > https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc > > (right): > > > > > https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:444: > > mutable RandomGenerator rand_gen_; > > This one looks shared between multiple instances, is it thread safe? > > > > > https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:903: > > if (test_->MaybeEndTest()) { > > Maybe rename MaybeEndTest to TestHasEnded? > > The RandomGenerator mutable might be a separate issue, not sure if this is a > regression. > > Thanks for the follow-ups I like this a bunch better. :) Thanks for the great feedback :-) The RandomGenerator is thread-safe. It is an object of a wrapper class that wraps a random generator with a lock.
https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:444: mutable RandomGenerator rand_gen_; On 2015/12/02 22:59:13, pbos-webrtc wrote: > This one looks shared between multiple instances, is it thread safe? It is, there us a custom class RandomGenerator which adds a lock around a test::Random random generator for this particular reason. https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:903: if (test_->MaybeEndTest()) { On 2015/12/02 22:59:13, pbos-webrtc wrote: > Maybe rename MaybeEndTest to TestHasEnded? That is a good name but I think it is a bit misleading. The problem is that the method has side-effects, and during a previous discussion in another similar test CL we settled on the current name instead for that particular reason. I'd be happy to go for another name, but then I think it should somehow reflect both the end-query and the actions of ending the test that the method does. Last time we thought the Maybe-name reflected that best. I'll keep the name for now.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1490113004/#ps60001 (title: "Added const specifiers and refactored the infinite waits in the threads")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490113004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490113004/60001
Message was sent while issue was closed.
Description was changed from ========== Rewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner. To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone. However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop. I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code. BUG= ========== to ========== Rewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner. To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone. However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop. I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Rewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner. To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone. However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop. I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code. BUG= ========== to ========== Rewrote the thread synchronization parts of the test for the locking in APM in response to a locking problem when running in a single-threaded manner. To try to resolve the problem I replaced the custom synchronization with rtc::Event which made the code cleaner, faster, and less error prone. However, in the end the source of the test locks was that during TearDown one of the threads was stuck in a waiting loop. I added a fix for the TearDown issue but still decided to keep the rtc:Event - based code change metioned above as that gave a more clean code. BUG= Committed: https://crrev.com/631e1345512c592eaebe19d9adeade6ee778624c Cr-Commit-Position: refs/heads/master@{#10880} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/631e1345512c592eaebe19d9adeade6ee778624c Cr-Commit-Position: refs/heads/master@{#10880}
Message was sent while issue was closed.
terelius@webrtc.org changed reviewers: + terelius@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:444: mutable RandomGenerator rand_gen_; On 2015/12/03 08:17:17, peah-webrtc wrote: > On 2015/12/02 22:59:13, pbos-webrtc wrote: > > This one looks shared between multiple instances, is it thread safe? > It is, there us a custom class RandomGenerator which adds a lock around a > test::Random random generator for this particular reason. RandomGenerator comes from base/helpers.h which is not in rtc_base_approved, though. Basically it is a wrapper around an SSL (or Windows API) implementation of a cryptographically secure RNG. Do you really need cryptographic security here?
Message was sent while issue was closed.
https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:444: mutable RandomGenerator rand_gen_; On 2015/12/09 17:10:38, terelius wrote: > On 2015/12/03 08:17:17, peah-webrtc wrote: > > On 2015/12/02 22:59:13, pbos-webrtc wrote: > > > This one looks shared between multiple instances, is it thread safe? > > It is, there us a custom class RandomGenerator which adds a lock around a > > test::Random random generator for this particular reason. > > RandomGenerator comes from base/helpers.h which is not in rtc_base_approved, > though. > > Basically it is a wrapper around an SSL (or Windows API) implementation of a > cryptographically secure RNG. Do you really need cryptographic security here? Oops, sorry. You are right. Confused it with a similar generator in base/helpers.h
Message was sent while issue was closed.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1490113004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:444: mutable RandomGenerator rand_gen_; On 2015/12/03 08:17:17, peah-webrtc wrote: > On 2015/12/02 22:59:13, pbos-webrtc wrote: > > This one looks shared between multiple instances, is it thread safe? > It is, there us a custom class RandomGenerator which adds a lock around a > test::Random random generator for this particular reason. Why share the PRNG? Why not just keep separate instances (with different seeds) in the different helper classes? |
