Add heap allocation to lib/support/Pool.h #11698
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
anush-apple,
balducci-apple,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
erjiaqing,
franck-apple,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
LuDuda,
mlepage-google,
mrjerryjohns,
msandstedt,
pan-apple,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux and
tecimovic
kpschoedel
force-pushed
the
x7715-pool-heap
branch
from
011139d to
ca2f2ed
Compare
|
PR #11698: Size comparison from b974630 to ca2f2ed Increases (36 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (14 builds for linux, mbed, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
msandstedt
reviewed
Nov 16, 2021
|
PR #11698: Size comparison from 09f9837 to 0c9ab88 Increases above 0.2%:
Increases (36 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (14 builds for linux, mbed, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
(#11880): Release all active objects (or verify that none are active) when destroying the pool.connectedhomeip/src/lib/support/Pool.h Lines 245 to 255 in 9d158a2 This comment was generated by todo based on a
|
|
PR #11698: Size comparison from 5a06fce to 9d158a2 Increases (36 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (25 builds for efr32, linux, mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
PR #11698: Size comparison from eb84a9d to b21add7 Increases (36 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (25 builds for efr32, linux, mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
PR #11698: Size comparison from e338b6e to 3758224 Increases (36 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (25 builds for efr32, linux, mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
andy31415
approved these changes
Nov 18, 2021
msandstedt
approved these changes
Nov 18, 2021
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 18, 2021
#### Problem
`chip::Server::Shutdown()` contains
```
chip::Dnssd::ServiceAdvertiser::Instance().Shutdown();
⋮
mCommissioningWindowManager.Cleanup();
```
but the latter restarts `Dnssd::ServiceAdvertiser`.
Instance of project-chip#11880 _Possible use of destroyed pool objects_
#### Change overview
Add `CommissioningWindowManager::Shutdown()` which does not restart DNS.
#### Testing
If `ObjectPool` checks that objects do not outlive it
(originally part of PR project-chip#11698 but deferred due to current leaks),
then `TestCommissionManager` fails without this change.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 18, 2021
#### Problem
`chip::Server::Shutdown()` contains
```
chip::Dnssd::ServiceAdvertiser::Instance().Shutdown();
⋮
mCommissioningWindowManager.Cleanup();
```
but the latter restarts `Dnssd::ServiceAdvertiser`.
Instance of project-chip#11880 _Possible use of destroyed pool objects_
#### Change overview
Add `CommissioningWindowManager::Shutdown()` which does not restart DNS.
#### Testing
If `ObjectPool` checks that objects do not outlive it
(originally part of PR project-chip#11698 but deferred due to current leaks),
then `TestCommissionManager` fails without this change.
kghost
approved these changes
Nov 19, 2021
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 19, 2021
#### Problem `echo_requester.cpp` leaks TCP endpoints. Instance of project-chip#11880 _Possible use of destroyed pool objects_ #### Change overview Shut down `TCPManager`. #### Testing If `ObjectPool` checks that objects do not outlive it (originally part of PR project-chip#11698 but deferred due to current leaks), then Cirque CI fails without this change.
kpschoedel
added a commit
that referenced
this pull request
Nov 21, 2021
#### Problem `echo_requester.cpp` leaks TCP endpoints. Instance of #11880 _Possible use of destroyed pool objects_ #### Change overview Shut down `TCPManager`. #### Testing If `ObjectPool` checks that objects do not outlive it (originally part of PR #11698 but deferred due to current leaks), then Cirque CI fails without this change.
Damian-Nordic
pushed a commit
that referenced
this pull request
Nov 22, 2021
#### Problem
`chip::Server::Shutdown()` contains
```
chip::Dnssd::ServiceAdvertiser::Instance().Shutdown();
⋮
mCommissioningWindowManager.Cleanup();
```
but the latter restarts `Dnssd::ServiceAdvertiser`.
Instance of #11880 _Possible use of destroyed pool objects_
#### Change overview
Add `CommissioningWindowManager::Shutdown()` which does not restart DNS.
#### Testing
If `ObjectPool` checks that objects do not outlive it
(originally part of PR #11698 but deferred due to current leaks),
then `TestCommissionManager` fails without this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We have too many pool allocators.
Previous PRs (#11428, #11487) transitionally use
BitMapObjectPoolwhere previously
System::ObjectPoolhad been used, but this lostthe ability to configure heap allocation.
Change overview
System::Timer(complementing Migrate System::Timer to BitMapObjectPool #11487)Co-authored-by: Zang MingJie zealot0630@gmail.com
Co-authored-by: C Freeman cecille@google.com
A future PR will use this for Inet pools (complementing #11428);
that is not done here because it would conflict with other Inet
changes under way.
Testing
Added heap versions of unit tests in TestPool. (A future PR will add a
System::Object-style statistics option and re-unify most of these tests.)CI should show
.bssdecreases corresponding to increases in #11487.