ozone/wayland: Add flag to use wayland thread with normal priority [chromium/src : main]
Tiago Vignatti (Gerrit)
Attention is currently required from: Maksim Sisov, Robert Kroeger.
Tiago Vignatti would like Maksim Sisov and Robert Kroeger to review this change.
ozone/wayland: Add flag to use wayland thread with normal priority
In ChromeOS, the current priority settings for Lacros processes right
now are misleading because only the Wayland event watcher thread
("wayland-fd") is running with high display priority. The ash-chrome
situation is different, with the following all set to use display
priority:
- wayland-fd thread (always enabled)
- browser UI and IO threads (enabled by BrowserUseDisplayThreadPriority
finch flag)
- GPU main, viz compositor and IO threads (enabled by
GpuUseDisplayThreadPriority)
- renderer compositor & IO threads (enabled by
BlinkCompositorUseDisplayThreadPriority)
It doesn't make sense to simply match-up Lacros settings with ash-chrome
(see crrev.com/c/3243266) because these systems are different by nature.
For example, critical user journey tests on Google Docs (benchmarked
locally) shows that setting wayland-fd thread to normal priority
improves page loading time in Lacros. So with a better understanding of
CPU scheduling and boost mechanisms, our desire next is to come up with
an efficient overall solution for threads priorities in Lacros.
Therefore, in order to test the effects of changing threads priorities,
this CL adds the ability to use wayland-fd with normal priority by
passing the --use-wayland-normal-thread-priority flag.
BUG=1262133
TEST=tast run $device lacros.DocsCUJ (modified to use the newly flag)
Change-Id: I113b60151b43e0ab112606a594bde2e7fdb97e84
---
M ui/ozone/public/ozone_switches.cc
M ui/ozone/platform/wayland/host/wayland_event_watcher.cc
M ui/ozone/public/ozone_switches.h
3 files changed, 55 insertions(+), 1 deletion(-)
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Tiago Vignatti (Gerrit)
Attention is currently required from: Maksim Sisov, Robert Kroeger.
1 comment:
Patchset:
PTAL.
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Tiago Vignatti (Gerrit)
Attention is currently required from: Maksim Sisov, Robert Kroeger.
Tiago Vignatti uploaded patch set #3 to this change.
ozone/wayland: Add flag to use wayland thread with normal priority
In ChromeOS, the current priority settings for Lacros processes right
now are misleading because only the Wayland event watcher thread
("wayland-fd") is running with high display priority. The ash-chrome
situation is different, with the following all set to use display
priority:
- wayland-fd thread (always enabled)
- browser UI and IO threads (enabled by BrowserUseDisplayThreadPriority
finch flag)
- GPU main, viz compositor and IO threads (enabled by
GpuUseDisplayThreadPriority)
- renderer compositor & IO threads (enabled by
BlinkCompositorUseDisplayThreadPriority)
It doesn't make sense to simply match-up Lacros settings with ash-chrome
(see crrev.com/c/3243266) because these systems are different by nature.
For example, critical user journey tests on Google Docs shows that
setting wayland-fd thread to normal priority improves page loading time
in Lacros (benchmarked locally). So with a better understanding of CPU
scheduling and boost mechanisms, our desire next is to come up with an
efficient overall solution for threads priorities in Lacros.
Therefore, in order to test the effects of changing threads priorities,
this CL adds the ability to use wayland-fd with normal priority by
passing the --use-wayland-normal-thread-priority flag.
BUG=1262133
TEST=tast run $device lacros.DocsCUJ (modified to use the newly flag)
Change-Id: I113b60151b43e0ab112606a594bde2e7fdb97e84
---
M ui/ozone/public/ozone_switches.cc
M ui/ozone/platform/wayland/host/wayland_event_watcher.cc
M ui/ozone/public/ozone_switches.h
3 files changed, 55 insertions(+), 1 deletion(-)
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Tiago Vignatti, Robert Kroeger.
Patch set 3:Code-Review +1
Robert Kroeger (Gerrit)
Attention is currently required from: Tiago Vignatti.
Patch set 3:Code-Review +1
2 comments:
Commit Message:
Patch Set #3, Line 15:
- wayland-fd thread (always enabled)Why is there a wayland-fd thread at all in Ash-Chrome? That seems wrong to me. Are you sure? And if you are: why? Why doesn't Ash-Chrome support the arrival of events at Exo on the I/O or UI threads?
File ui/ozone/platform/wayland/host/wayland_event_watcher.cc:
Patch Set #3, Line 101:
base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();It seems somewhat wrong that we don't specify this at the time of ::InitializeUI. Chrome has a somewhat regretful habit of using command line variables as convenient global variables. 😊
Can you add a TODO in the .h file to consider pruning the list of command line flags before landing.
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Tiago Vignatti (Gerrit)
Attention is currently required from: Robert Kroeger.
3 comments:
Commit Message:
Patch Set #3, Line 15:
- wayland-fd thread (always enabled)
Why is there a wayland-fd thread at all in Ash-Chrome? That seems wrong to me. […]
sorry, the description was wrong. I'm fixing it.
Patchset:
Thanks for review. One small question to address still, Robert.
File ui/ozone/platform/wayland/host/wayland_event_watcher.cc:
Patch Set #3, Line 101:
base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
It seems somewhat wrong that we don't specify this at the time of ::InitializeUI. […]
Are you suggesting to add a TODO in which .h file exactly, Robert?
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Tiago Vignatti (Gerrit)
Attention is currently required from: Robert Kroeger.
Tiago Vignatti uploaded patch set #4 to this change.
ozone/wayland: Add flag to use wayland thread with normal priority
In ChromeOS, the current priority settings for Lacros processes right
now are misleading because only the Wayland event watcher thread
("wayland-fd") is running with high display priority. The ash-chrome
situation is different, with the following all set to use display
priority:
- browser UI and IO threads (enabled by BrowserUseDisplayThreadPriority
finch flag)
- GPU main, viz compositor and IO threads (enabled by
GpuUseDisplayThreadPriority)
- renderer compositor & IO threads (enabled by
BlinkCompositorUseDisplayThreadPriority)
It doesn't make sense to simply match-up Lacros settings with ash-chrome
(see crrev.com/c/3243266) because these systems are different by nature.
For example, critical user journey tests on Google Docs shows that
setting wayland-fd thread to normal priority improves page loading time
in Lacros (benchmarked locally). So with a better understanding of CPU
scheduling and boost mechanisms, our desire next is to come up with an
efficient overall solution for threads priorities in Lacros.
Therefore, in order to test the effects of changing threads priorities
in Lacros, this CL adds the ability to use wayland-fd with normal
priority by passing the --use-wayland-normal-thread-priority flag.
BUG=1262133
TEST=tast run $device lacros.DocsCUJ (modified to use the newly flag)
Change-Id: I113b60151b43e0ab112606a594bde2e7fdb97e84
---
M ui/ozone/public/ozone_switches.cc
M ui/ozone/platform/wayland/host/wayland_event_watcher.cc
M ui/ozone/public/ozone_switches.h
3 files changed, 54 insertions(+), 1 deletion(-)
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Tiago Vignatti.
Patch set 4:Code-Review +1
1 comment:
File ui/ozone/platform/wayland/host/wayland_event_watcher.cc:
Patch Set #3, Line 101:
base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
Are you suggesting to add a TODO in which . […]
ozone_switches.h
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Tiago Vignatti (Gerrit)
2 comments:
Patchset:
Thanks!
File ui/ozone/platform/wayland/host/wayland_event_watcher.cc:
Patch Set #3, Line 101:
base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
ozone_switches. […]
Ack
To view, visit change 3304121. To unsubscribe, or for help writing mail filters, visit settings.
Tiago Vignatti (Gerrit)
Patch set 5:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ui/ozone/public/ozone_switches.h
Insertions: 2, Deletions: 0.
@@ -8,6 +8,8 @@
#include "base/compiler_specific.h"
#include "base/component_export.h"
+// TODO(rjkroege): Specify this at the time of ::InitializeUI to avoid the habit
+// of using command line variables as convenient as global variables.
namespace switches {
COMPONENT_EXPORT(OZONE_BASE) extern const char kOzonePlatform[];
```
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3304121
Commit-Queue: Tiago Vignatti <tvig...@igalia.com>
Reviewed-by: Robert Kroeger <rjkr...@chromium.org>
Reviewed-by: Maksim Sisov <msi...@igalia.com>
Cr-Commit-Position: refs/heads/main@{#946170}
---
M ui/ozone/public/ozone_switches.cc
M ui/ozone/platform/wayland/host/wayland_event_watcher.cc
M ui/ozone/public/ozone_switches.h
3 files changed, 61 insertions(+), 1 deletion(-)