Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2987933003:
Eliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a SequencedTaskChecker (Closed)
DescriptionEliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a SequencedTaskChecker
RtpVideoStreamReceiver::receive_cs_ is not really necessary, since all of the functions where that lock is acquired, are arrived at from functions of BaseChannel which DCHECK being called from BaseChannel::worker_thread_.
BUG=webrtc:8037
Review-Url: https://codereview.webrtc.org/2987933003
Cr-Commit-Position: refs/heads/master@{#19508}
Committed: https://chromium.googlesource.com/external/webrtc/+/8b07305b048fb91840ae7f9a58c6df7115540015
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase and change to a sequence-checker #Patch Set 3 : Rebased. #Patch Set 4 : Rebased #Patch Set 5 : Rebased #
Messages
Total messages: 28 (13 generated)
Description was changed from ========== Eliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a ThreadChecker RtpVideoStreamReceiver::receive_cs_ is not really necessary, since all of the functions where that lock is acquired, are arrived at from functions of BaseChannel which DCHECK being called from BaseChannel::worker_thread_. BUG=webrtc:8037 ========== to ========== Eliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a ThreadChecker RtpVideoStreamReceiver::receive_cs_ is not really necessary, since all of the functions where that lock is acquired, are arrived at from functions of BaseChannel which DCHECK being called from BaseChannel::worker_thread_. BUG=webrtc:8037 ==========
eladalon@webrtc.org changed reviewers: + danilchap@webrtc.org, solenberg@webrtc.org
Fredrick, do you think it's worth the effort to dig into the failing tests and try to fix them in order to remove this critical-section?
On 2017/07/26 18:43:17, eladalon wrote: > Fredrick, do you think it's worth the effort to dig into the failing tests and > try to fix them in order to remove this critical-section? I've seen before that tests use the Call API differently from the PC layer, so that may be worth looking into. It is surprising though that there only failures on iOS simulator - or are *all* the other bots you're using compile-only? Last time I profiled on a mobile device we'd spend as much as 7% acquiring locks, so yes, we want to get rid of them by moving to a more strict threading model.
On 2017/07/27 07:58:18, the sun wrote: > On 2017/07/26 18:43:17, eladalon wrote: > > Fredrick, do you think it's worth the effort to dig into the failing tests and > > try to fix them in order to remove this critical-section? > > I've seen before that tests use the Call API differently from the PC layer, so > that may be worth looking into. It is surprising though that there only failures > on iOS simulator - or are *all* the other bots you're using compile-only? > > Last time I profiled on a mobile device we'd spend as much as 7% acquiring > locks, so yes, we want to get rid of them by moving to a more strict threading > model. It's surprising indeed. I've used the same list of trybots I normally use, and those have (IIRC) yielded tests failures on video_engine_tests, etc., in the past.
for thread-related changes linux_tsan2 is a good tester. https://codereview.webrtc.org/2987933003/diff/1/webrtc/video/rtp_video_stream... File webrtc/video/rtp_video_stream_receiver.cc (right): https://codereview.webrtc.org/2987933003/diff/1/webrtc/video/rtp_video_stream... webrtc/video/rtp_video_stream_receiver.cc:115: has_received_frame_(false) { does constructor always run on the worker thread too? if not, you might want to detach the thread checker here.
https://codereview.webrtc.org/2987933003/diff/1/webrtc/video/rtp_video_stream... File webrtc/video/rtp_video_stream_receiver.cc (right): https://codereview.webrtc.org/2987933003/diff/1/webrtc/video/rtp_video_stream... webrtc/video/rtp_video_stream_receiver.cc:115: has_received_frame_(false) { On 2017/07/27 10:24:59, danilchap wrote: > does constructor always run on the worker thread too? > if not, you might want to detach the thread checker here. It always runs on it for non-tests, AFAIK.
Description was changed from ========== Eliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a ThreadChecker RtpVideoStreamReceiver::receive_cs_ is not really necessary, since all of the functions where that lock is acquired, are arrived at from functions of BaseChannel which DCHECK being called from BaseChannel::worker_thread_. BUG=webrtc:8037 ========== to ========== Eliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a SequencedTaskChecker RtpVideoStreamReceiver::receive_cs_ is not really necessary, since all of the functions where that lock is acquired, are arrived at from functions of BaseChannel which DCHECK being called from BaseChannel::worker_thread_. BUG=webrtc:8037 ==========
eladalon@webrtc.org changed reviewers: + stefan@webrtc.org
Patchset #2 (id:20001) has been deleted
I've rebased this on top of a CL that resolves the issues that were failing the tests. Please take another look.
lgtm
Ping @stefan.
Nice! lgtm
The CQ bit was checked by eladalon@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2987933003/#ps100001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
The CQ bit was checked by eladalon@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1503647031051280,
"parent_rev": "8cee56f2546c329d193d9478e29894cf9f6ad2ff", "commit_rev":
"8b07305b048fb91840ae7f9a58c6df7115540015"}
Message was sent while issue was closed.
Description was changed from ========== Eliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a SequencedTaskChecker RtpVideoStreamReceiver::receive_cs_ is not really necessary, since all of the functions where that lock is acquired, are arrived at from functions of BaseChannel which DCHECK being called from BaseChannel::worker_thread_. BUG=webrtc:8037 ========== to ========== Eliminate RtpVideoStreamReceiver::receive_cs_ in favor of using a SequencedTaskChecker RtpVideoStreamReceiver::receive_cs_ is not really necessary, since all of the functions where that lock is acquired, are arrived at from functions of BaseChannel which DCHECK being called from BaseChannel::worker_thread_. BUG=webrtc:8037 Review-Url: https://codereview.webrtc.org/2987933003 Cr-Commit-Position: refs/heads/master@{#19508} Committed: https://chromium.googlesource.com/external/webrtc/+/8b07305b048fb91840ae7f9a5... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/8b07305b048fb91840ae7f9a5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
