New issue
Advanced search Search tips

Issue 631915 link

Starred by 2 users
Status: Duplicate
Merged: issue 631416
Owner: ----
Closed: Jul 2016
Cc:
rdevlin....@chromium.org
brucedaw...@chromium.org
esprehn@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

extensions::APIFeatureProvider::APIFeatureProvider compiles to 200KB of code

Project Member Reported by dcheng@chromium.org, Jul 27 2016 
In an official build, this function compiles into 200KB of code. The function (from generated code: https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/extensions/api/api_features.cc?rcl=0&l=18) is fairly large, but this is surprisingly large!

Similarly, extensions::PermissionFeatureProvider::PermissionFeatureProvider compiles into 150+KBof code.

I looked at the disassembly and a large portion appears to be the result of constructing std::strings. However, the disassembly is also interesting. Starting at 0x160553, there's a huge block of jumps which all look very similar:

   0x0000000001a1f0c9 <+160553>:	add    $0xfffffffffffffff8,%rax
   0x0000000001a1f0cd <+160557>:	cmpq   $0x0,0x4aaaec3(%rip)        # 0x64c9f98
   0x0000000001a1f0d5 <+160565>:	je     0x1a25929 <extensions::APIFeatureProvider::APIFeatureProvider()+187273>
   0x0000000001a1f0db <+160571>:	mov    $0xffffffff,%ecx
   0x0000000001a1f0e0 <+160576>:	lock xadd %ecx,(%rax)
   0x0000000001a1f0e4 <+160580>:	jmpq   0x1a25930 <extensions::APIFeatureProvider::APIFeatureProvider()+187280>
   0x0000000001a1f0e9 <+160585>:	add    $0xfffffffffffffff8,%rax
   0x0000000001a1f0ed <+160589>:	cmpq   $0x0,0x4aaaea3(%rip)        # 0x64c9f98
   0x0000000001a1f0f5 <+160597>:	je     0x1a2594d <extensions::APIFeatureProvider::APIFeatureProvider()+187309>
   0x0000000001a1f0fb <+160603>:	mov    $0xffffffff,%ecx
   0x0000000001a1f100 <+160608>:	lock xadd %ecx,(%rax)
   0x0000000001a1f104 <+160612>:	jmpq   0x1a25954 <extensions::APIFeatureProvider::APIFeatureProvider()+187316>
   0x0000000001a1f109 <+160617>:	add    $0xfffffffffffffff8,%rax
   0x0000000001a1f10d <+160621>:	cmpq   $0x0,0x4aaae83(%rip)        # 0x64c9f98
   0x0000000001a1f115 <+160629>:	je     0x1a2596b <extensions::APIFeatureProvider::APIFeatureProvider()+187339>
   0x0000000001a1f11b <+160635>:	mov    $0xffffffff,%ecx
   0x0000000001a1f120 <+160640>:	lock xadd %ecx,(%rax)
   0x0000000001a1f124 <+160644>:	jmpq   0x1a25972 <extensions::APIFeatureProvider::APIFeatureProvider()+187346>

Incidentally, this is the last 40KB of the function or so.

I think we can probably use a combination of StringPiece / const char* to get some easy codesize wins here, but I'm wondering if there are potential codegen improvements to make in clang here.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 28 2016 

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

commit d8b8acff3d2d57e66b833a5632e3a993eb405277
Author: dcheng <dcheng@chromium.org>
Date: Thu Jul 28 07:04:02 2016

Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before. In all,
this saves about 275KB of binary size in an official Linux GN build.

BUG= 631915 

Review-Url: https://codereview.chromium.org/2186893002
Cr-Commit-Position: refs/heads/master@{#408350}

[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/extensions/common/features/base_feature_provider.cc
[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/extensions/common/features/base_feature_provider.h
[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/extensions/common/features/feature.cc
[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/extensions/common/features/feature.h
[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/extensions/common/features/json_feature_provider.cc
[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/extensions/common/features/simple_feature.cc
[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/extensions/common/features/simple_feature.h
[modify] https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277/tools/json_schema_compiler/feature_compiler.py

Comment 2 by brucedaw...@chromium.org, Jul 29 2016 

Nice work.

Perhaps the compiler should have avoided doing so much inlining, but making calls to lots of STL functions is also not cheap, so this seems better.

I assume this saved code space on other platforms as well?

Comment 3 by dcheng@chromium.org, Jul 29 2016 

Yeah, this helped Mac and Windows as well. See the graphs on https://chromeperf.appspot.com/report?sid=9401ee2b031ddfa3bcde52eae6cf602d840150a0088911fd315fa1d50aa3e5fa (I'm not sure how to specify a revision range, but r407500+ was where size increased by a bunch, and r408350+ is where it decreased.

That being said, it didn't seem to help Windows as much, so there might be other codegen issues there. Unfortunately, I can't currently investigate on Windows. Also, see  issue 631416 , which is where the perf sheriffs noticed this increase.

Comment 4 by dcheng@chromium.org, Jul 29 2016

Mergedinto: 631416
Status: Duplicate (was: Untriaged)
I'm going to dupe this into 631416 so we can track the followup work on Windows there.
►

Sign in to add a comment