New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 760250 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression



Sign in to add a comment

40kb regression in resource_sizes (MonochromePublic.apk) at 497934:497934

Project Member Reported by agrieve@chromium.org, Aug 29 2017

Issue description

Caused by “Add out of process gpu raster behind a flag”

Commit: 436a77572ce1088aff858c11afe43c9198655c46

Link to size graph: https://chromeperf.appspot.com/report?sid=a097e74b1aa288511afb4cb616efe0f95ba4d347ad61d5e835072f23450938ba&num_points=10&rev=497934

Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase

Based on the graph, increase is entirely from native code.

 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Aug 29 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=760250

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=2f45e11272c49bacc74c124e5a6ee9f5587224149ae86b0dfa4a1f9e953fdb44


Bot(s) for this bug's original alert(s):

Android Builder
Note: 40kb is somewhat sizeable. Please put in a bit of effort to verify that:
1) This code is required on Android
2) There is no easy way to make it smaller.

Attached the diff from diagnose_bloat.py below. From it, I can see:
* The top 10 symbols account for 35kb out of 40kb
* The first symbol is likely not needed on Android (which supports only GLES afaik)

Section Sizes (Total=39.6kb (40580 bytes)):
    .bss: 0 bytes (0 bytes) (not included in totals)
    .data: 0 bytes (0 bytes) (0.0%)
    .data.rel.ro: 288 bytes (288 bytes) (0.7%)
    .rel.dyn: -3.45kb (-3536 bytes) (-8.7%)
    .rodata: 4.50kb (4608 bytes) (11.4%)
    .text: 38.3kb (39216 bytes) (96.6%)

415 symbols added (+), 23 changed (~), 117 removed (-), 521111 unchanged (not shown)
Number of unique symbols 402054 -> 402180 (+126)
3 paths added, 0 removed, 17 changed

Showing 555 symbols (aliases not grouped for diffs) with total pss: 44112 bytes
.text=38.3kb     .rodata=4.50kb     .data.rel.ro=288 bytes  .data=0 bytes    .bss=0 bytes    total=43.1kb
Number of unique paths: 31

Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss
Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path
------------------------------------------------------------
+ 0)      14128 (31.9%) t@0x686480   +14128 (0->14128)  third_party/skia/src/gpu/gl/GrGLAssembleInterface.cpp
               GrGLAssembleGLInterface
+ 1)      24184 (54.6%) t@0x683d38   +10056 (0->10056)  third_party/skia/src/gpu/gl/GrGLAssembleInterface.cpp
               GrGLAssembleGLESInterface
~ 2)      28911 (65.3%) r@Group      +4727 (2979763->2984490) {{no path}}
               ** merge strings (count=7)
+ 3)      30545 (69.0%) t@0x8800c2   +1634 (0->1634)    cc/paint/paint_op_reader.cc
               cc::PaintOpReader::Read
+ 4)      31797 (71.8%) t@0x880c52   +1252 (0->1252)    cc/paint/paint_op_writer.cc
               cc::PaintOpWriter::Write
+ 5)      32753 (74.0%) t@0x1891884  +956 (0->956)      gpu/command_buffer/service/gles2_cmd_decoder.cc
               gpu::gles2::GLES2DecoderImpl::DoBeginRasterCHROMIUM
~ 6)      33225 (75.0%) t@0x9fa954   +472 (528->1000)   cc/raster/gpu_raster_buffer_provider.cc
               cc::GpuRasterBufferProvider::PlaybackOnWorkerThread
+ 7)      33685 (76.1%) t@0x87fba4   +460 (0->460)      cc/paint/paint_op_reader.cc
               cc::PaintOpReader::Read
+ 8)      34121 (77.1%) t@0x880a9e   +436 (0->436)      cc/paint/paint_op_writer.cc
               cc::PaintOpWriter::Write
+ 9)      34539 (78.0%) t@0x87f2ce   +418 (0->418)      cc/paint/paint_op_buffer.cc
               cc::PaintOpBuffer::FlatteningIterator::FlattenCurrentOpIfNeeded
+ 10)     34895 (78.8%) t@0xa811e4   +356 (0->356)      gpu/command_buffer/client/gles2_implementation.cc
               gpu::gles2::GLES2Implementation::RasterCHROMIUM
+ 11)     35231 (79.6%) t@0x5332c8   +336 (0->336)      third_party/skia/src/core/SkPathRef.cpp
               SkPathRef::isValid const
+ 12)     35539 (80.3%) t@0x880724   +308 (0->308)      cc/paint/paint_op_reader.cc
               cc::PaintOpReader::Read
+ 13)     35837 (80.9%) t@0x1874700  +298 (0->296)      gpu/command_buffer/service/gles2_cmd_decoder.cc
               gpu::gles2::GLES2DecoderImpl::HandleRasterCHROMIUM
+ 14)     36093 (81.5%) t@0x87d5d4   +256 (0->256)      cc/paint/paint_op_buffer.cc
               cc::DrawImageRectOp::Deserialize
+ 15)     36341 (82.1%) t@0x881136   +248 (0->248)      cc/paint/paint_op_writer.cc
               cc::PaintOpWriter::Write
+ 16)     36571 (82.6%) t@0x2117788  +230 (0->228)      content/renderer/render_thread_impl.cc
               content::CreateOffscreenContext
- 17)     36349 (82.1%) t@0x0        -222 (220->0)      content/renderer/render_thread_impl.cc
               content::CreateOffscreenContext
~ 18)     36129 (81.6%) t@Group      -220 (0->0)        {{no path}}
               ** symbol gaps (count=9)
~ 19)     36337 (82.1%) t@0x1876c20  +208 (5600->5808)  gpu/command_buffer/service/gles2_cmd_decoder.cc
               gpu::gles2::GLES2DecoderImpl::Initialize
+ 20)     36525 (82.5%) t@0x87d230   +188 (0->188)      cc/paint/paint_op_buffer.cc
               cc::AnnotateOp::Deserialize
+ 21)     36709 (82.9%) t@0x87d2ec   +184 (0->184)      cc/paint/paint_op_buffer.cc
               cc::ClipPathOp::Deserialize
+ 22)     36879 (83.3%) t@0x87fd70   +170 (0->170)      cc/paint/paint_op_reader.cc
               cc::PaintOpReader::ReadFlattenable
+ 23)     37049 (83.7%) t@0x87fe1a   +170 (0->170)      cc/paint/paint_op_reader.cc
               cc::PaintOpReader::ReadFlattenable
+ 24)     37219 (84.0%) t@0x87fec4   +170 (0->170)      cc/paint/paint_op_reader.cc
               cc::PaintOpReader::ReadFlattenable
+ 25)     37389 (84.4%) t@0x87ff6e   +170 (0->170)      cc/paint/paint_op_reader.cc
               cc::PaintOpReader::ReadFlattenable
+ 26)     37559 (84.8%) t@0x880018   +170 (0->170)      cc/paint/paint_op_reader.cc
               cc::PaintOpReader::ReadFlattenable
+ 27)     37729 (85.2%) t@0x87dad8   +170 (0->168)      cc/paint/paint_op_buffer.cc
               cc::SaveLayerOp::Deserialize
+ 28)     37897 (85.6%) t@0x9fb072   +168 (0->168)      cc/raster/gpu_raster_buffer_provider.cc
               cc::DisplayItemList::push (num_aliases=2)
+ 29)     38063 (86.0%) t@0x87d848   +166 (0->166)      cc/paint/paint_op_buffer.cc
               cc::DrawPathOp::Deserialize
+ 30)     38225 (86.3%) t@0x880858   +162 (0->162)      cc/paint/paint_op_writer.cc
               cc::PaintOpWriter::WriteFlattenable
+ 31)     38385 (86.7%) t@0x87d9ee   +160 (0->160)      cc/paint/paint_op_buffer.cc
               cc::DrawTextBlobOp::Deserialize
+ 32)     38535 (87.0%) t@0x87d7b4   +150 (0->148)      cc/paint/paint_op_buffer.cc

Comment 3 by enne@chromium.org, Aug 29 2017

Cc: vmi...@chromium.org
vmiura: it looks like 60% of this is coming from GrGLAssembleInterface.  Is there some alternative on Android that is less binary size, do you think?

I don't really want to #ifdef this patch out on Android, since it would really only be a short term thing.  Android seems one of the more important platforms for this work, especially with enabling Vulkan.

Comment 4 by vmi...@chromium.org, Aug 29 2017

Cc: piman@chromium.org bsalomon@chromium.org
Can we avoid linking in GrGLAssembleGLInterface on Android?  Keep only GrGLAssembleGLESInterface?


On the client side, Brian implemented CreateGLES2InterfaceBindings which was more efficient than going via GrGLAssembleGLESInterface, but now I guess we have both getting linked.

Roughly an option is to refactor CreateGLES2InterfaceBindings to use GrGLAssembleGLESInterface, which means making a GetProc equivalent for GLES2Interface.  A table based approach could take less space.

Comment 5 by piman@chromium.org, Aug 29 2017

We already look up most (all?) these functions in ui/gl/gl_bindings*, so there may be a cheaper way to do this, by accessing gl::g_current_gl_driver (which is a gl::DriverGL*, which has fields for all these functions). It would avoid the string lookups and calls to the GetProcAddress function, and even the extension/version tests (since we also do those in the bindings) - it "should" be possible to make it as small as copying N function pointers.

Comment 6 by vmi...@chromium.org, Aug 29 2017

CreateGLES2InterfaceBindings is only 4KB; adding a GetProc isn't likely to save anything so please ignore that suggestion.

Implementing GrGLAssembleGLESInterface in terms of g_current_gl_driver should in turn get us to ~4KB.

Comment 7 by piman@chromium.org, Aug 29 2017

Cc: enne@chromium.org
Owner: piman@chromium.org
Let me take a look.

Comment 8 by piman@chromium.org, Aug 30 2017

Tentatively a quick hack shows that we should be able to save ~23kb with the approach from #5. I say tentatively because it doesn't fully work yet.

Comment 9 by piman@chromium.org, Sep 1 2017

https://chromium-review.googlesource.com/c/chromium/src/+/646957 is a somewhat principled attempts, with some caveats.

It probably won't land before the branch, we can decide whether it's better to merge that or a trivial patch that disables the feature.
Labels: -Pri-2 ReleaseBlock-Beta Pri-1
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/148d0597cb0d72a73c3cc2e481396cb6b24cc535

commit 148d0597cb0d72a73c3cc2e481396cb6b24cc535
Author: Antoine Labour <piman@chromium.org>
Date: Fri Sep 01 21:00:20 2017

Create GrGLInterface based on ui/gl's bindings instead of GrGLAssembleGLInterface

GrGLAssembleGLInterface is a lot of code to lookup GL function pointers,
which is somewhat duplicated in ui/gl's bindings. This saves about 21kb
on Android.

Bug:  760250 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I906997ab8cc03db06a437d3428ca4263b13d2025
Reviewed-on: https://chromium-review.googlesource.com/646957
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499293}
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/gpu/command_buffer/service/BUILD.gn
[add] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/gpu/command_buffer/service/create_gr_gl_interface.cc
[add] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/gpu/command_buffer/service/create_gr_gl_interface.h
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/generate_bindings.py
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/gl_bindings_api_autogen_gl.h
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/gl_bindings_autogen_gl.cc
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/gl_bindings_autogen_gl.h
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/gl_bindings_autogen_mock.cc
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/gl_bindings_autogen_mock.h
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/gl_mock_autogen_gl.h
[modify] https://crrev.com/148d0597cb0d72a73c3cc2e481396cb6b24cc535/ui/gl/gl_stub_autogen_gl.h

I have another indirectly related change that helps saving another 15k: https://chromium-review.googlesource.com/c/chromium/src/+/648244
It should also help us scale better as we add support for more extensions.
Project Member

Comment 13 by sheriffbot@chromium.org, Sep 4 2017

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20d1620b86ae574a79cad73116d7c1f339ddabd7

commit 20d1620b86ae574a79cad73116d7c1f339ddabd7
Author: Antoine Labour <piman@chromium.org>
Date: Tue Sep 05 23:05:29 2017

Introduce gl::ExtensionSet and simplify extension handling in GL bindings

- ExtensionSet is just a flat_map of StringPiece's. After construction, we only
  look up into ExtensionSet, so a flat map is ideal. Using StringPiece avoids
  copies.
- Remove GLContext::GetExtensions platform overrides. They used to add
  WGL/GLX/EGL extensions to the GL extension string, but we never test for them in
  that path (i.e. that's pure overhead). If we need to expose them in the future,
  a separate function would be preferable.
- Have GLContext::GetExtensions compute, cache, and return the ExtensionSet.
  This avoids extraneous string manipulations (split/join) and copies.

As a side effect, bindings now lookup the actual extension string as opposed to
adding a space after it. This allows sharing the string with other places we
test for those extensions. It also makes the bindings initialization code much
more efficient. About 15Kb saved on Android.

Bug:  760250 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: If830612ca7e23f705808c7b0908322671553122c
Reviewed-on: https://chromium-review.googlesource.com/648244
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499783}
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/gl_context_virtual.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/gl_context_virtual.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/shader_translator_unittest.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/test_helper.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/test_helper.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/command_buffer/service/texture_manager_unittest.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/gpu/config/gpu_info_collector.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/BUILD.gn
[add] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/extension_set.cc
[add] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/extension_set.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/generate_bindings.py
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_api_unittest.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_bindings.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_bindings_autogen_egl.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_bindings_autogen_gl.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_bindings_autogen_glx.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_bindings_autogen_osmesa.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_bindings_autogen_wgl.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_egl.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_egl.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_glx.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_glx.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_stub.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_stub.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_wgl.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_context_wgl.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_implementation.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_implementation.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_version_info.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_version_info.h
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/gl_version_info_unittest.cc
[modify] https://crrev.com/20d1620b86ae574a79cad73116d7c1f339ddabd7/ui/gl/init/gl_factory_android.cc

Project Member

Comment 15 by sheriffbot@chromium.org, Sep 8 2017

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 16 by piman@chromium.org, Sep 11 2017

Labels: Merge-Request-62
Requesting merge for https://chromium-review.googlesource.com/646957. It's mostly harmless because it essentially only touches a path that is disabled by default.

We could consider https://chromium-review.googlesource.com/648244 too for extra savings, depending on appetite. It is more risky in that it touches production code. It landed safely though (AFAICT at this point).
Please add appropriate OSs.

Comment 18 by piman@chromium.org, Sep 11 2017

Labels: OS-Android
Project Member

Comment 19 by sheriffbot@chromium.org, Sep 12 2017

Labels: -Merge-Request-62 Hotlist-Merge-Approved Merge-Approved-62
Your change meets the bar and is auto-approved for M62. Please go ahead and merge the CL to branch 3202 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 20 by bugdroid1@chromium.org, Sep 12 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd

commit e1d02623bdb26e3ddc14b32879bf471bffcbe3cd
Author: Antoine Labour <piman@chromium.org>
Date: Tue Sep 12 18:46:36 2017

Create GrGLInterface based on ui/gl's bindings instead of GrGLAssembleGLInterface

GrGLAssembleGLInterface is a lot of code to lookup GL function pointers,
which is somewhat duplicated in ui/gl's bindings. This saves about 21kb
on Android.

Bug:  760250 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I906997ab8cc03db06a437d3428ca4263b13d2025
Reviewed-on: https://chromium-review.googlesource.com/646957
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499293}(cherry picked from commit 148d0597cb0d72a73c3cc2e481396cb6b24cc535)
Reviewed-on: https://chromium-review.googlesource.com/663669
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#176}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/gpu/command_buffer/service/BUILD.gn
[add] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/gpu/command_buffer/service/create_gr_gl_interface.cc
[add] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/gpu/command_buffer/service/create_gr_gl_interface.h
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/generate_bindings.py
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/gl_bindings_api_autogen_gl.h
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/gl_bindings_autogen_gl.cc
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/gl_bindings_autogen_gl.h
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/gl_bindings_autogen_mock.cc
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/gl_bindings_autogen_mock.h
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/gl_mock_autogen_gl.h
[modify] https://crrev.com/e1d02623bdb26e3ddc14b32879bf471bffcbe3cd/ui/gl/gl_stub_autogen_gl.h

Project Member

Comment 21 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4076bacedad5df5611ce7d7f26f164f46d3cbb8c

commit 4076bacedad5df5611ce7d7f26f164f46d3cbb8c
Author: Antoine Labour <piman@chromium.org>
Date: Tue Sep 12 18:48:22 2017

Introduce gl::ExtensionSet and simplify extension handling in GL bindings

- ExtensionSet is just a flat_map of StringPiece's. After construction, we only
  look up into ExtensionSet, so a flat map is ideal. Using StringPiece avoids
  copies.
- Remove GLContext::GetExtensions platform overrides. They used to add
  WGL/GLX/EGL extensions to the GL extension string, but we never test for them in
  that path (i.e. that's pure overhead). If we need to expose them in the future,
  a separate function would be preferable.
- Have GLContext::GetExtensions compute, cache, and return the ExtensionSet.
  This avoids extraneous string manipulations (split/join) and copies.

As a side effect, bindings now lookup the actual extension string as opposed to
adding a space after it. This allows sharing the string with other places we
test for those extensions. It also makes the bindings initialization code much
more efficient. About 15Kb saved on Android.

Bug:  760250 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: If830612ca7e23f705808c7b0908322671553122c
Reviewed-on: https://chromium-review.googlesource.com/648244
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499783}(cherry picked from commit 20d1620b86ae574a79cad73116d7c1f339ddabd7)
Reviewed-on: https://chromium-review.googlesource.com/663883
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#177}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/gl_context_virtual.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/gl_context_virtual.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/shader_translator_unittest.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/test_helper.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/test_helper.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/command_buffer/service/texture_manager_unittest.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/gpu/config/gpu_info_collector.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/BUILD.gn
[add] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/extension_set.cc
[add] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/extension_set.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/generate_bindings.py
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_api_unittest.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_bindings.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_bindings_autogen_egl.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_bindings_autogen_gl.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_bindings_autogen_glx.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_bindings_autogen_osmesa.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_bindings_autogen_wgl.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_egl.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_egl.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_glx.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_glx.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_stub.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_stub.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_wgl.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_context_wgl.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_implementation.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_implementation.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_version_info.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_version_info.h
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/gl_version_info_unittest.cc
[modify] https://crrev.com/4076bacedad5df5611ce7d7f26f164f46d3cbb8c/ui/gl/init/gl_factory_android.cc

Comment 22 by piman@chromium.org, Sep 12 2017

Status: Fixed (was: Assigned)
Merged both patches per offline discussion with aluo@

Sign in to add a comment