Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471073002:
X11: Use CopyFromParent colormap when possible (Reland) (Closed)
|
Created:
4 years, 1 month ago by Tom (Use chromium acct) Modified:
4 years, 1 month ago CC:
chromium-reviews, derat+watch_chromium.org, sadrul, tfarina, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionX11: Use CopyFromParent colormap when possible (Reland)
This CL is a small performance optimization. Previously,
XVisualManager always allocated a new colormap for every used visual.
However, on non-compositing WMs, on some drivers, this could result in
an expensive copy-on-present for every frame, but this should not
occur when using the root window's colormap.
BUG=661697
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel
Committed: https://crrev.com/66edd15df4d85fa3ee13e8dd911bee8ffcfbe885
Cr-Commit-Position: refs/heads/master@{#430050}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add comment, init default_visual_id_ #Patch Set 3 : Fix segfault in Angle #
Total comments: 1
Patch Set 4 : Don't return CopyFromParent for visual or depth #
Messages
Total messages: 40 (23 generated)
thomasanderson@google.com changed reviewers: + derat@chromium.org, erg@chromium.org
erg + derat PTAL erg: DWTHX11 derat: x11_util*
The CQ bit was checked by thomasanderson@google.com 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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for x11_util.* https://codereview.chromium.org/2471073002/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/2471073002/diff/1/ui/base/x/x11_util.cc#newco... ui/base/x/x11_util.cc:1423: system_visual_id_(0), nit: also initialize default_visual_id_ to 0 here for consistency https://codereview.chromium.org/2471073002/diff/1/ui/base/x/x11_util.cc#newco... ui/base/x/x11_util.cc:1441: system_visual_id_ = default_visual_id_; i was initially confused about why there are two members with the same value, but then i saw that system_visual_id_ can be updated by XVisualManager::OnGPUInfoChanged.
https://codereview.chromium.org/2471073002/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/2471073002/diff/1/ui/base/x/x11_util.cc#newco... ui/base/x/x11_util.cc:1423: system_visual_id_(0), On 2016/11/02 15:14:46, Daniel Erat wrote: > nit: also initialize default_visual_id_ to 0 here for consistency Done. https://codereview.chromium.org/2471073002/diff/1/ui/base/x/x11_util.cc#newco... ui/base/x/x11_util.cc:1441: system_visual_id_ = default_visual_id_; On 2016/11/02 15:14:46, Daniel Erat wrote: > i was initially confused about why there are two members with the same value, > but then i saw that system_visual_id_ can be updated by > XVisualManager::OnGPUInfoChanged. Added a small comment in x11_util_internal.h to clarify this
lgtm Please add a bug# so we can track this patch as it gets landed, reverted because it breaks weird system X and relanded.
Description was changed from ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. ========== to ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 ==========
On 2016/11/02 18:01:58, Elliot Glaysher wrote: > lgtm > > Please add a bug# so we can track this patch as it gets landed, reverted because > it breaks weird system X and relanded. done
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2471073002/#ps20001 (title: "Add comment, init default_visual_id_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 ========== to ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 ========== to ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 Committed: https://crrev.com/a03a18c3b24025623e0f2ad96ac56b8427fda7d3 Cr-Commit-Position: refs/heads/master@{#429371} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a03a18c3b24025623e0f2ad96ac56b8427fda7d3 Cr-Commit-Position: refs/heads/master@{#429371}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2471403002/ by ynovikov@chromium.org. The reason for reverting is: This causes crashes on Linux with --use-gl=angle. https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Debug%20%28New... https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28N... https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Debug%20%28NVI... Crashing stack: #0 XVisualIDFromVisual (visual=0x0) at ../../src/Misc.c:60 #1 0x00007f6ab2e4a7bb in GetPlatformANGLEDisplay () at ../../ui/gl/gl_surface_egl.cc:153 #2 0x00007f6ab2e482a8 in GetDisplayFromType () at ../../ui/gl/gl_surface_egl.cc:170 #3 InitializeDisplay () at ../../ui/gl/gl_surface_egl.cc:621 #4 0x00007f6ab2e47d37 in InitializeOneOff () at ../../ui/gl/gl_surface_egl.cc:471 #5 0x00007f6ab5cad2d5 in InitializeGLOneOffPlatform () at ../../ui/gl/init/gl_initializer_x11.cc:141 #6 0x00007f6ab5cad0d6 in InitializeGLOneOffImplementation () at ../../ui/gl/init/gl_factory.cc:65 #7 0x00007f6ab5cace70 in InitializeGLOneOff () at ../../ui/gl/init/gl_factory.cc:56 #8 0x00007f6ab5e46c2e in InitializeAndStartSandbox () at ../../gpu/ipc/service/gpu_init.cc:178 #9 0x00007f6ab42ba16b in GpuMain () at ../../content/gpu/gpu_main.cc:250 #10 0x00007f6ab1eb5267 in RunNamedProcessTypeMain () at ../../content/app/content_main_runner.cc:408 #11 0x00007f6ab1eb5cdb in Run () at ../../content/app/content_main_runner.cc:776 #12 0x00007f6ab1eb4650 in ContentMain () at ../../content/app/content_main.cc:20 #13 0x00007f6ab0819c5d in ChromeMain () at ../../chrome/app/chrome_main.cc:97 #14 0x00007f6aa92d0f45 in __libc_start_main (main=0x7f6ab0819c10 <main>, argc=17, argv=0x7ffd9a4f1468, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffd9a4f1458) at libc-start.c:287 #15 0x00007f6ab0819b31 in _start () .
Message was sent while issue was closed.
Description was changed from ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 Committed: https://crrev.com/a03a18c3b24025623e0f2ad96ac56b8427fda7d3 Cr-Commit-Position: refs/heads/master@{#429371} ========== to ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 Committed: https://crrev.com/a03a18c3b24025623e0f2ad96ac56b8427fda7d3 Cr-Commit-Position: refs/heads/master@{#429371} ==========
Description was changed from ========== X11: Use CopyFromParent colormap when possible This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 Committed: https://crrev.com/a03a18c3b24025623e0f2ad96ac56b8427fda7d3 Cr-Commit-Position: refs/heads/master@{#429371} ========== to ========== X11: Use CopyFromParent colormap when possible (Reland) This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel ==========
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
thomasanderson@google.com changed reviewers: + sadrul@chromium.org
+sadrul for ui/gl/gl_surface_egl.cc
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/03 at 17:45:56, thomasanderson wrote: > +sadrul for ui/gl/gl_surface_egl.cc One comment, that might avoid having to do changes to gl_surface_egl.
cwallez@chromium.org changed reviewers: + cwallez@chromium.org
https://codereview.chromium.org/2471073002/diff/40001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/2471073002/diff/40001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:152: if (visual == CopyFromParent) How about making XVisualManager only return CopyFromParent for the colormap? Wouldn't that be sufficient for the purpose of this patch?
On 2016/11/03 21:16:56, Corentin Wallez wrote: > https://codereview.chromium.org/2471073002/diff/40001/ui/gl/gl_surface_egl.cc > File ui/gl/gl_surface_egl.cc (right): > > https://codereview.chromium.org/2471073002/diff/40001/ui/gl/gl_surface_egl.cc... > ui/gl/gl_surface_egl.cc:152: if (visual == CopyFromParent) > How about making XVisualManager only return CopyFromParent for the colormap? > Wouldn't that be sufficient for the purpose of this patch? sgtm, done Will try to land after a dry run unless there are any objections
thomasanderson@google.com changed reviewers: - sadrul@chromium.org
On 2016/11/04 at 17:31:44, thomasanderson wrote: > On 2016/11/03 21:16:56, Corentin Wallez wrote: > > https://codereview.chromium.org/2471073002/diff/40001/ui/gl/gl_surface_egl.cc > > File ui/gl/gl_surface_egl.cc (right): > > > > https://codereview.chromium.org/2471073002/diff/40001/ui/gl/gl_surface_egl.cc... > > ui/gl/gl_surface_egl.cc:152: if (visual == CopyFromParent) > > How about making XVisualManager only return CopyFromParent for the colormap? > > Wouldn't that be sufficient for the purpose of this patch? > > sgtm, done > Will try to land after a dry run unless there are any objections patch 4 lgtm, thanks!
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2471073002/#ps60001 (title: "Don't return CopyFromParent for visual or depth")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== X11: Use CopyFromParent colormap when possible (Reland) This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel ========== to ========== X11: Use CopyFromParent colormap when possible (Reland) This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== X11: Use CopyFromParent colormap when possible (Reland) This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel ========== to ========== X11: Use CopyFromParent colormap when possible (Reland) This CL is a small performance optimization. Previously, XVisualManager always allocated a new colormap for every used visual. However, on non-compositing WMs, on some drivers, this could result in an expensive copy-on-present for every frame, but this should not occur when using the root window's colormap. BUG=661697 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel Committed: https://crrev.com/66edd15df4d85fa3ee13e8dd911bee8ffcfbe885 Cr-Commit-Position: refs/heads/master@{#430050} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/66edd15df4d85fa3ee13e8dd911bee8ffcfbe885 Cr-Commit-Position: refs/heads/master@{#430050} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
