Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745813003:
Don't set the priority of the decoder to 'high' on Android. (Closed)
|
Created:
3 years, 9 months ago by tommi Modified:
3 years, 9 months ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't set the priority of the decoder to 'high' on Android.
Doing so competes with the actual decoding that happens on a different thread.
BUG=695438
Review-Url: https://codereview.webrtc.org/2745813003
Cr-Commit-Position: refs/heads/master@{#17184}
Committed: https://chromium.googlesource.com/external/webrtc/+/ca37cf669174184b5139ffd0141bf7f6c0fc7894
Patch Set 1 #
Messages
Total messages: 17 (8 generated)
tommi@webrtc.org changed reviewers: + stefan@webrtc.org
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
I'm surprised that it actually is possible to set the thread priority on Android. Have you verified that it makes a difference? If so I think we should really consider what priorities we are setting, as I suspect many of our assumptions may not hold...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/11 11:35:27, stefan-webrtc wrote: > I'm surprised that it actually is possible to set the thread priority on > Android. Have you verified that it makes a difference? If so I think we should > really consider what priorities we are setting, as I suspect many of our > assumptions may not hold... (Replying in cr) I haven't verified. The code is there though and I have an idea for what we need to do, but it requires several changes. One way to see if this has an effect, is to land and see if anything happens :-)
On 2017/03/11 12:20:04, tommi (webrtc) wrote: > On 2017/03/11 11:35:27, stefan-webrtc wrote: > > I'm surprised that it actually is possible to set the thread priority on > > Android. Have you verified that it makes a difference? If so I think we should > > really consider what priorities we are setting, as I suspect many of our > > assumptions may not hold... > > (Replying in cr) > > I haven't verified. The code is there though and I have an idea for what we need > to do, but it requires several changes. > One way to see if this has an effect, is to land and see if anything happens :-) As a side note, I think we're overusing thread priorities at the moment. We're always competing with the UI thread, which is a normal priority thread. We also have a dedicated thread per video streams which for big meetings on desktop, we can have more high priority threads only for decoding than there are cores on the system. Yesterday I was looking at a dump with 47 decoder threads for example. (It's extreme, but it's where we're heading)
On 2017/03/11 12:27:21, tommi (webrtc) wrote: > On 2017/03/11 12:20:04, tommi (webrtc) wrote: > > On 2017/03/11 11:35:27, stefan-webrtc wrote: > > > I'm surprised that it actually is possible to set the thread priority on > > > Android. Have you verified that it makes a difference? If so I think we > should > > > really consider what priorities we are setting, as I suspect many of our > > > assumptions may not hold... > > > > (Replying in cr) > > > > I haven't verified. The code is there though and I have an idea for what we > need > > to do, but it requires several changes. > > One way to see if this has an effect, is to land and see if anything happens > :-) > > As a side note, I think we're overusing thread priorities at the moment. We're > always competing with the UI thread, which is a normal priority thread. We also > have a dedicated thread per video streams which for big meetings on desktop, we > can have more high priority threads only for decoding than there are cores on > the system. Yesterday I was looking at a dump with 47 decoder threads for > example. (It's extreme, but it's where we're heading) Agree. I'm not even convinced we should be using high prio threads. I'm open for landing this to see if it makes any difference. lgtm
On 2017/03/11 12:40:23, stefan-webrtc wrote: > On 2017/03/11 12:27:21, tommi (webrtc) wrote: > > On 2017/03/11 12:20:04, tommi (webrtc) wrote: > > > On 2017/03/11 11:35:27, stefan-webrtc wrote: > > > > I'm surprised that it actually is possible to set the thread priority on > > > > Android. Have you verified that it makes a difference? If so I think we > > should > > > > really consider what priorities we are setting, as I suspect many of our > > > > assumptions may not hold... > > > > > > (Replying in cr) > > > > > > I haven't verified. The code is there though and I have an idea for what we > > need > > > to do, but it requires several changes. > > > One way to see if this has an effect, is to land and see if anything happens > > :-) > > > > As a side note, I think we're overusing thread priorities at the moment. We're > > always competing with the UI thread, which is a normal priority thread. We > also > > have a dedicated thread per video streams which for big meetings on desktop, > we > > can have more high priority threads only for decoding than there are cores on > > the system. Yesterday I was looking at a dump with 47 decoder threads for > > example. (It's extreme, but it's where we're heading) > > Agree. I'm not even convinced we should be using high prio threads. > > I'm open for landing this to see if it makes any difference. > > lgtm Great, I'll watch the perf bots.
The CQ bit was checked by tommi@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": 1, "attempt_start_ts": 1489236706657990, "parent_rev":
"d118bacc9b854817eaaeb947b8c058941b432d38", "commit_rev":
"ca37cf669174184b5139ffd0141bf7f6c0fc7894"}
Message was sent while issue was closed.
Description was changed from ========== Don't set the priority of the decoder to 'high' on Android. Doing so competes with the actual decoding that happens on a different thread. BUG=695438 ========== to ========== Don't set the priority of the decoder to 'high' on Android. Doing so competes with the actual decoding that happens on a different thread. BUG=695438 Review-Url: https://codereview.webrtc.org/2745813003 Cr-Commit-Position: refs/heads/master@{#17184} Committed: https://chromium.googlesource.com/external/webrtc/+/ca37cf669174184b5139ffd01... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/ca37cf669174184b5139ffd01...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.webrtc.org/2757733005/ by tommi@webrtc.org. The reason for reverting is: Reverting this as it had no effect.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
