FeatureProvider constructors could be much smaller |
||||||
Issue description
Three of the largest functions in Chrome (64-bit Windows canary, 64.0.3244.0) are generated code coming from .json templates:
63830 code public: __cdecl extensions::APIFeatureProvider::APIFeatureProvider(void) __ptr64 c:\b\c\b\win64_pgo\src\out\release_x64\gen\chrome\common\extensions\api\api_features.cc
52008 code public: __cdecl extensions::PermissionFeatureProvider::PermissionFeatureProvider(void) __ptr64 c:\b\c\b\win64_pgo\src\out\release_x64\gen\chrome\common\extensions\api\permission_features.cc
24593 code public: __cdecl extensions::ManifestFeatureProvider::ManifestFeatureProvider(void) __ptr64 c:\b\c\b\win64_pgo\src\out\release_x64\gen\chrome\common\extensions\api\manifest_features.cc
These three constructors feature somewhat boilerplate code blocks like this:
{
PermissionFeature* feature = new PermissionFeature();
feature->set_name("accessibilityFeatures.modify");
feature->set_channel(version_info::Channel::STABLE);
feature->set_extension_types({Manifest::TYPE_EXTENSION,Manifest::TYPE_PLATFORM_APP});
AddFeature("accessibilityFeatures.modify", feature);
}
It looks like the size of the code could be dramatically reduced by emitting tables of data and loops instead of so much repeated code. This could be particularly important on Android. These functions are large enough that shrinking them could improve startup performance by reducing disk I/O requirements.
,
Oct 20 2017
,
Oct 20 2017
Also see issue 631416 . It used to be over 400KB =) Using data tables would definitely help, but the last time I looked at this, I recall thinking that would be at least somewhat tricky due to the fact that we have SimpleFeature and ComplexFeature and they can be nested. This gives me an idea though...
,
Oct 20 2017
,
Oct 20 2017
It would also be nice if the PermissionFeature objects were const static instead of allocated, to reduce private data, but that may not be practical.
,
Oct 20 2017
dcheng@, lemme know if I can help in any way!
,
Oct 26 2017
OK I spent longer than I was planning poking at this... I stubbed out enough code to get an idea of the binary size impact. Unfortunately, the results aren't great. I'm unassigning myself for now in case someone wants to poke at this. On Linux x64: In a plain release build with dchecks off, the baseline size is 134,546,424 bytes. In a naive implementation of data tables [1], the size is 134,606,568 bytes. Switching to more compact representations [2]... gets the size down to 134,541,240 bytes. This has to do with the fact that SimpleFeatureConfig just has too many slots that aren't always used. There are a few fields that are rarely used (<10): - blacklist - command_line_switch - component_extensions_auto_granted - max_manifest_version Both component_extensions_auto_granted and max_manifest_version are already packed, so removing them has minimal effect on size. Removing blacklist and command_line_switch brings the size down to 134,527,608 bytes, at the cost of increasing generator and feature provider complexity. An alternate implementation strategy that might work better is to unroll the tables in a different dimension: we could store separate tables about all the blacklists, all the command line switches, all the dependencies, et cetera. The overhead would be higher the more features a given attribute is set for though. [1] https://chromium-review.googlesource.com/c/chromium/src/+/731703/2 [2] https://chromium-review.googlesource.com/c/chromium/src/+/731703/4
,
Oct 26 2017
I built official this morning as well for comparison, and the relative numbers are about the same on Linux x64: 121,373,312 is the starting point 121,354,432 is the ending point (using bitmasks and out-of-lining rarely used attributes) I didn't do a Windows build but I wouldn't expect a large delta there.
,
Oct 26 2017
Did you manage to come up with a fully constexpr way of defining the data? If so then even if the binary size difference is miniscule there would also be a drop in the amount of allocated data which would probably be more significant. That is, if we go from 200 KB of code that is allocating memory to ~200 KB of read-only data then our binary size will not change but we are in a strictly better place. Can you clarify this?
,
Oct 26 2017
I believe the data is fully constexpr (in that the compiler accepts it), but I didn't verify that it all ends up in rodata. Note that PS4 is required to be competitive with current sizes today, and it isn't fully implemented. If someone wants to pick it up before I get back to it, the remaining work items: - Figure out how to suppress the large inlined ctor warning, since we're using aggregate initialization anyway. One easy (but cheating) way is to move it to a folder with "/gen/" in the name =) - Define bitmask values and add conversion routines from bitmasks to actual enums (the actual enums can't change, since they're used for UMA). - Modify feature_compiler.py to emit bitmasks for lists of enums - Update SimpleFeature(const FeatureConfig&) to use bitmask conversion routines - Implement FeatureProvider::AddFeaturesFromConfig(). For further size improvements, we'd want to drop std::initializer_list altogether, at the cost of making the generator even more complex. AFAIK, none of the lists are longer than 255 entries, so using 8 bytes for size is overkill. The nice part about doing this is it'd make SimpleFeatureConfig and ComplexFeatureConfig PODs again--and then we don't need to trick the clang plugin into not emitting the warning. The downside is that the generator will have to explicitly define arrays to hold the data, and refer to those arrays when constructing the feature configs. If we want to make the code extra fun, we can probably even pack the size and the pointer to the array in the same word, given that the address space is limited to 48 bits today. This sounds a bit too horrible to seriously consider though =)
,
Oct 27 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by agrieve@google.com
, Oct 20 2017