Make ConfigurationManager virtual #10195
Conversation
|
|
|
Currently, the Device Layer uses CRTP design pattern to make it easier to adapt the code to different platforms and operating contexts. As part of its design, the CHIP Device Layer employs a pattern of static polymorphism to insulate its application-visible API from the underlying platform-specific implementation. A similar interface pattern is used within the Device Layer itself to provide compartmentalization between components. Using static polymorphism instead of dynamic polymorphism (virtual function) is mainly based memory consideration. Since virtual function is extensively used within other core components, I think it make sense to look at if we need to switch PlatformLayer also to virtual functions. Lets see the memory consumption change with this PR. |
harimau-qirex
force-pushed
the
virtual-configuration-manager
branch
from
4642935 to
57d93e6
Compare
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
Damian-Nordic,
erjiaqing,
franck-apple,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
LuDuda and
mlepage-google
harimau-qirex
force-pushed
the
virtual-configuration-manager
branch
from
f04ac10 to
b19f315
Compare
|
PR #10195: Size comparison from efc17de to b19f315 1 build
Increases above 1.0% from efc17de to b19f315:
19 builds
Increases above 1.0% from efc17de to b19f315:
14 builds
|
|
PR #10195: Size comparison from efc17de to ebd0ae4 Increases above 1.0% from efc17de to ebd0ae4:
8 builds
Increases above 1.0% from efc17de to ebd0ae4:
14 builds
2 builds
Increases above 1.0% from efc17de to ebd0ae4:
10 builds
|
|
I've run the diffsyms.py script and investigated what's causing the size increase (with the Linux chip-tool as a first check). The difference is largely due to the fact that many ConfigurationManager (and Impl) symbols are optimized out of the master build because they don't get used, and that leads to other non-ConfigurationManager symbols that also get removed because now nothing uses them. When the ConfigurationManager is virtual, these calls can't get optimized out and so the full cost is paid for them. The top three symbols that were included with this change that aren't included on master are: These alone account for a bit less than 18 kB. There are about twenty related to config-value storage (PosixConfig, ChipLinuxStorage, ChipLinuxStorageIni) that are about another 8 kB. The unused ConfigurationManager methods add up to about another 8 kB. The remaining few kB generally seem to be directly related to the virtualization (1248 B for vtables, plus increases to ConfigurationMgr() call sites). |
|
Now that #10528 has been merged would it make sense to update this PR and check sizes again? |
Hi Andy, I was wondering if we can optimize the flash further. If we decide to proceed with virtual function route, then we need to convert all platform classes from CRTP to virtual functions, the add-up cost of flash increase could be huge. After we switching to virtual interface, I think we should be able to clean up some files from current hierarchy, such as GenericConfigurationMgrImpl.h/GenericConfigurationMgrImpl.cpp since ConfigurationMgr can define the default implementation by itself. We just need to have each platforms define their own ConfigurationMgrImpl class which inherits from ConfigurationMgr class and override the functions they need. |
|
Yufeng: Andrei: For methods that were optimized out:
The first case is unavoidable and the cost will need to be paid, either by this pull request or by someone else's in the future when it gets used. The second case is addressed by the part of the plan to create domain-specific APIs rather than having everything rely on the grab-bag that is ConfigurationManager. Depending on how those APIs are organised, it should be possible to either not include or let the optimizer remove the unused code, much like it is now. The third case may be resolved by removing the method entirely (i.e. expand on the work that #10528 started), or by moving the definition to the implementation. For example, values that are read-only in the regular build but set in a factory build only need to have setters in the factory build's implementation. The regular build's implementation can simply not include setters since they aren't needed. |
There was a problem hiding this comment.
Approving as an intermediate path to splitting configuration manager into more focused intefaces. This is based on expectations of:
(a) size increase is very large only for linux and we understand it (pulls in networking code for things like 'get mac address)
(b) we expect to gain back size splitting into intefaces and removing intermediate layes of config manager.
Yes, the intermediate GenericConfigurationManagerImpl layer was introduced only to implement CRTP pattern, we don't need this layer after switching to virtual interface. We can remove this layer as a whole, I am not sure how much memory we can save from this change, but it will make the ConfigurationMgr much easier to understand. It looks we only need to convert ConfigurationMgr to virtual interface to have type aware API, so the memory increase seems still within our control. |
harimau-qirex
force-pushed
the
virtual-configuration-manager
branch
from
ebd0ae4 to
bef469d
Compare
To make it possible to set ConfigurationManager at the start of the program, the next step is to make the interface pure virtual. The generic implementation also needs to inherit from this interface. Each platform's implementation is updated to work with the new interface.
The next step to making ConfigurationManager virtual is to remove the use of Impl() in GenericConfigurationManagerImpl, since the virtual methods already handle delegating to the inheritor. Impl() can't be completely removed yet because it's also used for other functions not part of the ConfigurationManager interface.
…h the new virtual ConfigurationManager interface.
…ter reflect the spec.
harimau-qirex
force-pushed
the
virtual-configuration-manager
branch
from
bef469d to
5506077
Compare
|
Size increase report for "gn_qpg-example-build" from 40f6345
Full report output |
|
Size increase report for "nrfconnect-example-build" from 40f6345
Full report output |
|
Size increase report for "esp32-example-build" from 40f6345
Full report output |
Problem
Making the configuration manager virtual will make it easier to change at runtime. Later changes can allow the ConfigurationMgr() accessor to be settable at runtime rather than a hardcoded static instance.
Change overview
Makes the ConfigurationManager interface pure virtual. GenericConfigurationManagerImpl now implements it directly, and calls on the Impl() for the ConfigurationManager methods have been removed. Other calls remain for interfaces not defined by the ConfigurationManager (e.g. the methods related to config values).
Testing
Ran the unit tests.