Inet/(TCP/UDP)Endpoints: Migrate from SystemPool to BitMapObjectPool #11428
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
yufengwangca
previously requested changes
Nov 4, 2021
There was a problem hiding this comment.
I think the consensus is to migrate all pool management to dynamic based pool, not the other way around.
We have spent huge effort to convert system pool to support dynamic allocation which can save significant memory usage on enabled platforms, we should not simply dump all of our those works without clear justification.
|
PR #11428: Size comparison from 1538bbb to 01f4929 Increases above 0.2%:
Increases (18 builds for linux, nrfconnect, telink)
Decreases (18 builds for linux, nrfconnect, telink)
Full report (19 builds for linux, nrfconnect, telink)
|
|
After sync-up, this PR is the transitional change. #11428 is the transitional plan and Inet will temporarily use BitMapObjectPool and soon use the unified implementation with the same dynamic implementation option as now. |
|
PR #11428: Size comparison from abcae0d to a6ec444 Increases above 0.2%:
Increases (13 builds for efr32, k32w, p6, qpg, telink)
Decreases (12 builds for efr32, k32w, p6, qpg, telink)
Full report (13 builds for efr32, k32w, p6, qpg, telink)
|
pullapprove
bot
requested review from
andy31415,
anush-apple,
balducci-apple,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
franck-apple,
hawk248,
holbrookt and
jelderton
|
It looks like the +328 or +632 byes memory usage on variety platforms are due to The TCP endpoint pool is not linked previously because there is no TCP usage. |
msandstedt
reviewed
Nov 5, 2021
msandstedt
reviewed
Nov 5, 2021
msandstedt
reviewed
Nov 5, 2021
msandstedt
reviewed
Nov 5, 2021
msandstedt
reviewed
Nov 5, 2021
msandstedt
reviewed
Nov 5, 2021
There was a problem hiding this comment.
Comments noted inline.
In general, this seems like it makes sense. The retain/release calls on the two sides of the queue make sense and simplifies things.
I think LwIPHandleIncomingConnection could use rework though. There's way to much reference count manipulation happening there. I honestly can't tell whether the code is correct.
msandstedt
approved these changes
Nov 5, 2021
kpschoedel
approved these changes
Nov 8, 2021
jelderton
approved these changes
Nov 8, 2021
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 9, 2021
#### Problem `Inet::TCPEndPoint` and `Inet::UDPEndPoint` historically could not use constructors because of `System::ObjectPool` limitations. Incidentally renamed `mAppState` for consistency (it had been in `System::Object` prior to project-chip#11428). This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_, split off to reduce the complexity of an upcoming PR. #### Change overview Convert from `Init()` to constructors. Transitionally, the constructors still call a per-implementation function `InitImpl()`. #### Testing CI; no changes to functionality.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 9, 2021
#### Problem `Inet::TCPEndPoint` and `Inet::UDPEndPoint` historically could not use constructors because of `System::ObjectPool` limitations. Incidentally renamed `mAppState` for consistency (it had been in `System::Object` prior to project-chip#11428). This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_, split off to reduce the complexity of an upcoming PR. #### Change overview Convert from `Init()` to constructors. Transitionally, the constructors still call a per-implementation function `InitImpl()`. #### Testing CI; no changes to functionality.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 9, 2021
#### Problem `Inet::TCPEndPoint` and `Inet::UDPEndPoint` historically could not use constructors because of `System::ObjectPool` limitations. Incidentally renamed `mAppState` for consistency (it had been in `System::Object` prior to project-chip#11428). This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_, split off to reduce the complexity of an upcoming PR. #### Change overview Convert from `Init()` to constructors. Transitionally, the constructors still call a per-implementation function `InitImpl()`. #### Testing CI; no changes to functionality.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 9, 2021
#### Problem `Inet::TCPEndPoint` and `Inet::UDPEndPoint` historically could not use constructors because of `System::ObjectPool` limitations. Incidentally renamed `mAppState` for consistency (it had been in `System::Object` prior to project-chip#11428). This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_, split off to reduce the complexity of an upcoming PR. #### Change overview Convert from `Init()` to constructors. Transitionally, the constructors still call a per-implementation function `InitImpl()`. #### Testing CI; no changes to functionality.
andy31415
pushed a commit
that referenced
this pull request
Nov 10, 2021
* Inet: Use constructors for Inet EndPoints #### Problem `Inet::TCPEndPoint` and `Inet::UDPEndPoint` historically could not use constructors because of `System::ObjectPool` limitations. Incidentally renamed `mAppState` for consistency (it had been in `System::Object` prior to #11428). This is a step toward #7715 _Virtualize System and Inet interfaces_, split off to reduce the complexity of an upcoming PR. #### Change overview Convert from `Init()` to constructors. Transitionally, the constructors still call a per-implementation function `InitImpl()`. #### Testing CI; no changes to functionality. * explicitly initialize all members
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 11, 2021
#### Problem We have too many pool allocators. Previous PRs (project-chip#11428, project-chip#11487) transitionally use `BitMapObjectPool` where previously `System::ObjectPool` had been used, but this lost the ability to configure heap allocation. #### Change overview - Add a heap allocator (from project-chip#9590) - Add allocation selection (from project-chip#11371) - Use this for `System::Timer` (complementing project-chip#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 project-chip#11428); that is not done here because it would conflict with other Inet changes under way. #### Testing CI; no changes to functionality. CI should show `.bss` decreases corresponding to increases in project-chip#11487.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 11, 2021
#### Problem We have too many pool allocators. Previous PRs (project-chip#11428, project-chip#11487) transitionally use `BitMapObjectPool` where previously `System::ObjectPool` had been used, but this lost the ability to configure heap allocation. #### Change overview - Add a heap allocator (from project-chip#9590) - Add allocation selection (from project-chip#11371) - Use this for `System::Timer` (complementing project-chip#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 project-chip#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 `System::Object`-style statistics and re-unify most of these tests.) CI should show `.bss` decreases corresponding to increases in project-chip#11487.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 15, 2021
#### Problem We have too many pool allocators. Previous PRs (project-chip#11428, project-chip#11487) transitionally use `BitMapObjectPool` where previously `System::ObjectPool` had been used, but this lost the ability to configure heap allocation. #### Change overview - Add a heap allocator (from project-chip#9590) - Add allocation selection (from project-chip#11371) - Use this for `System::Timer` (complementing project-chip#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 project-chip#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 `System::Object`-style statistics and re-unify most of these tests.) CI should show `.bss` decreases corresponding to increases in project-chip#11487.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 16, 2021
#### Problem We have too many pool allocators. Previous PRs (project-chip#11428, project-chip#11487) transitionally use `BitMapObjectPool` where previously `System::ObjectPool` had been used, but this lost the ability to configure heap allocation. #### Change overview - Add a heap allocator (from project-chip#9590) - Add allocation selection (from project-chip#11371) - Use this for `System::Timer` (complementing project-chip#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 project-chip#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 `System::Object`-style statistics and re-unify most of these tests.) CI should show `.bss` decreases corresponding to increases in project-chip#11487.
kpschoedel
added a commit
to kpschoedel/connectedhomeip
that referenced
this pull request
Nov 16, 2021
#### Problem We have too many pool allocators. Previous PRs (project-chip#11428, project-chip#11487) transitionally use `BitMapObjectPool` where previously `System::ObjectPool` had been used, but this lost the ability to configure heap allocation. #### Change overview - Add a heap allocator (from project-chip#9590) - Add allocation selection (from project-chip#11371) - Use this for `System::Timer` (complementing project-chip#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 project-chip#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 `System::Object`-style statistics and re-unify most of these tests.) CI should show `.bss` decreases corresponding to increases in project-chip#11487.
kpschoedel
added a commit
that referenced
this pull request
Nov 19, 2021
#### Problem We have too many pool allocators. Previous PRs (#11428, #11487) transitionally use `BitMapObjectPool` where previously `System::ObjectPool` had been used, but this lost the ability to configure heap allocation. #### Change overview - Add a heap allocator (from #9590) - Add allocation selection (from #11371) - Use this for `System::Timer` (complementing #11487) - Factor out common code. - Use a heap-allocated list to track heap-allocated objects. - More unit tests. 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 `System::Object`-style statistics and re-unify most of these tests.) CI should show `.bss` decreases corresponding to increases in #11487.
PSONALl
pushed a commit
to PSONALl/connectedhomeip
that referenced
this pull request
Dec 3, 2021
…roject-chip#11428) * Migrate inet/endpoints to BitMapPool * Remove atomic
PSONALl
pushed a commit
to PSONALl/connectedhomeip
that referenced
this pull request
Dec 3, 2021
* Inet: Use constructors for Inet EndPoints #### Problem `Inet::TCPEndPoint` and `Inet::UDPEndPoint` historically could not use constructors because of `System::ObjectPool` limitations. Incidentally renamed `mAppState` for consistency (it had been in `System::Object` prior to project-chip#11428). This is a step toward project-chip#7715 _Virtualize System and Inet interfaces_, split off to reduce the complexity of an upcoming PR. #### Change overview Convert from `Init()` to constructors. Transitionally, the constructors still call a per-implementation function `InitImpl()`. #### Testing CI; no changes to functionality. * explicitly initialize all members
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
Use BitMapObjectPool as basic pool implementation, deprecate
SystemPool, will introduce a dynamic pool later, sharing same interface asBitMapObjectPoolfor Linux and Darwin platformChange overview
SystemPooltoBitMapObjectPoolScheduleLambdainstead ofPostEvent, remove inet event dispatcherRetainbefore queuing the endpoint into the event queue, callReleaseafter the event is processed, removeDeferralFreeTesting
Verified using unit-tests