Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate ObjectPool into static and dynamic parts #11371

Merged
merged 3 commits into from
Nov 5, 2021

Conversation

cecille
Copy link
Contributor

@cecille cecille commented Nov 3, 2021

Problem

The general goal of this refactor is to separate the static and
dynamic parts of the ObjectPool so that developers that support
heap allocation can also opt to use static allocation for portions
of their code. This refactor maps ObjectPool to the same default
types as current (dynamic if heap allocation is allowed, static
otherwise).

Upcoming refactor PRs:

  • adding unlocked iterator access so we can use range-based for
    loops (works more easily with the design of certain part of the
    SDK stack)
  • Demonstrating the use of the unsized abstract base type pointer
    for passing pools of unknown size

Change overview

Separates the static and dynamic portions of ObjectPool into two classes and provides a default mapping to ObjectPool based on the config setting.

Testing

ObjectPool tests pass. Also affects inet tests (also passing).

Copy link
Contributor

@yufengwangca yufengwangca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the purpose of this refactor is not to allow developer to use dynamic and static pool simultaneously. This decision should be made based on the platform capability and invisible to developers.

@kpschoedel
Copy link
Contributor

I think the purpose of this refactor is not to allow developer to use dynamic and static pool simultaneously. This decision should be made based on the platform capability and invisible to developers.

So, this came up again because I ran into a painful inconsistency in ObjectPool: in the heap implementation, the object constructor gets called, but in the static implementation it doesn't.

I'm agnostic as to whether the programmer should be able to force a static pool, but in practice they can and do today by using BitMapObjectPool. That is mostly a nicer static pool than this one, since it doesn't require objects to inherit from System::Object, and (like the heap ObjectPool) does invoke the constructor/destructor.

BitMapObjectPool however doesn't provide Retain/Release. That is used only by TCPEndPoint.

There's the additional question of synchronization. The CHIP rule is that the SDK is not thread-safe, although it's not (yet) clear that all uses behave correctly. In Weave, the Object synchronization was required internally by AsyncDNS, but we don't have that. (BitMapObjectPool does synchronize.)

What I propose:

  • System::Object goes away.
  • The heap allocator here moves to Pool.h and gets the same API as BitMapObjectPool, i.e. not requiring Object as first superclass.
  • Move Retain/Release to TCPEndPoint (probably via a mixin).

@msandstedt msandstedt self-requested a review November 4, 2021 01:50
@msandstedt
Copy link
Contributor

I support @kpschoedel's suggestions. The reference counting is one of the most difficult things here. If we can move this into the class or classes that actually need it, that would simplify things a lot.

Copy link
Contributor

@msandstedt msandstedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this will be good to go once @bzbarsky-apple 's comments are addressed.

@andy31415
Copy link
Contributor

/rebase

cecille and others added 3 commits November 5, 2021 15:07
The general goal of this refactor is to separate the static and
dynamic parts of the ObjectPool so that developers that support
heap allocation can also opt to use static allocation for portions
of their code. This refactor maps ObjectPool to the same default
types as current (dynamic if heap allocation is allowed, static
otherwise).

Upcoming refactor PRs:
- adding unlocked iterator access so we can use range-based for
  loops (works more easily with the design of certain part of the
  SDK stack)
- Demonstrating the use of the unsized abstract base type pointer
  for passing pools of unknown size
Previous size_t changes broke the build - just using size_t now
for all stats.

Also remove the base class. The idea with that was to allow this
pool to be used with the unsigned base classes in the mdns code.
However, it looks like it's causing memory bloat despite the fact
that nothing actually uses that base class. So it looks like
maybe we just need to refactor to mdns.
@woody-apple woody-apple force-pushed the separate_static_dynamic branch from 366710b to 98097eb Compare November 5, 2021 15:07
@woody-apple woody-apple merged commit 09eb37b into project-chip:master Nov 5, 2021
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 <[email protected]>
Co-authored-by: C Freeman <[email protected]>

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 <[email protected]>
Co-authored-by: C Freeman <[email protected]>

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 <[email protected]>
Co-authored-by: C Freeman <[email protected]>

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 <[email protected]>
Co-authored-by: C Freeman <[email protected]>

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 <[email protected]>
Co-authored-by: C Freeman <[email protected]>

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 <[email protected]>
Co-authored-by: C Freeman <[email protected]>

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
* Separate ObjectPool into static and dynamic parts

The general goal of this refactor is to separate the static and
dynamic parts of the ObjectPool so that developers that support
heap allocation can also opt to use static allocation for portions
of their code. This refactor maps ObjectPool to the same default
types as current (dynamic if heap allocation is allowed, static
otherwise).

Upcoming refactor PRs:
- adding unlocked iterator access so we can use range-based for
  loops (works more easily with the design of certain part of the
  SDK stack)
- Demonstrating the use of the unsized abstract base type pointer
  for passing pools of unknown size

* Apply suggestions from code review

Co-authored-by: Michael Sandstedt <[email protected]>

* Fix size_t and remove base class.

Previous size_t changes broke the build - just using size_t now
for all stats.

Also remove the base class. The idea with that was to allow this
pool to be used with the unsigned base classes in the mdns code.
However, it looks like it's causing memory bloat despite the fact
that nothing actually uses that base class. So it looks like
maybe we just need to refactor to mdns.

Co-authored-by: Michael Sandstedt <[email protected]>
@cecille cecille deleted the separate_static_dynamic branch April 1, 2022 00:02
@cecille cecille restored the separate_static_dynamic branch April 1, 2022 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants