Merge SystemPool and support/Pool

[kghost]

Problem

We have too many pool implementation.

Change overview

There are 3 changes:

Add a PostLambda to post a generic lambda into system event queue #9678

Add a PostLambda to post a generic lambda into system event queue. This will decouple system object from network endpoints (TCPEndpoint, UDPEndpoint).

There is several limitation of the lambda

• The lambda must be trivially copyable. Because the lambda is memcpy into the queue. It means that the lambda can only capture pod data (Including pointers).
• The size of the lambda must less than the queue element size. The size of the lambda is the sum of all captured fields. The body of the lambda is encoded in the type info of the lambda, which is captured by a static function LambdaProxy::Call, and resolved into a function pointer at compile time, assigned to LambdaBridge::LambdaProxy into the queue element.

Unify SystemPool and lib/support/Pool #9590

Unify the interface of system/SystemPool and lib/support/pool. This PR change the interface of SystemPool to match lib/support/Pool. The pool provides following interface functions:

    T * CreateObject(Args&&... args)
    void ReleaseObject(T * object)
    template <typename Function> bool ForEachActiveObject(Function && function)

And also provides statistics interface:

    void ResetStatistics() {}
    void GetStatistics(Stats::count_t & aNumInUse, Stats::count_t & aHighWatermark) {}

Following features are decoupled from the pool implementation:

• Reference counter: if the target require this feature, it can derive from lib/core/ReferenceCounted class
• DeferredRelease: this feature should be orthogonal to pool implementation, the target class should implement it by itself.

Files changes:
Removed

• src/system/SystemObject.cpp
• src/system/SystemObject.h

Implementation on platforms with heap (POSIX):

• src/system/SystemPoolHeap.h

Implementation on platforms w/o heap (Embedded):

• src/system/SystemPoolNonHeap.h
• src/system/SystemPoolNonHeapBitmapAllocator.cpp
• src/system/SystemPoolNonHeapBitmapAllocator.h

Statistics:

• src/system/SystemPoolStatistics.h

Select implementation based on pre-defined macro

• src/system/SystemPool.h

Merge SystemPool and support/Pool #9748 (this PR)

Remove lib/support/Pool, migrate all its usage to system/SystemPool

Testing

Verified using unit tests

[msandstedt]

Please don't use recursive mutexes.

@mspang ,

The recursive mutex comes about with the original object pool design which was more than an allocator: it also allowed the sdk to iterate over all allocated objects in the pool. So instead of properly tracking state, we shoe-horned this iteration facility into an allocator to allow the sdk to do a foreach on all objects of a given class without actually having to actively track anything.

When we moved to the heap implementation, this became a very difficult problem. And now to do an object release or mutation in the middle of the foreach loop, you need the recursive mutex.

Is there a way that we can remove the need to iterate over all pool objects in the first place? This would make things much, much simpler.

[mspang]

Please don't use recursive mutexes.

@mspang ,

The recursive mutex comes about with the original object pool design which was more than an allocator: it also allowed the sdk to iterate over all allocated objects in the pool. So instead of properly tracking state, we shoe-horned this iteration facility into an allocator to allow the sdk to do a foreach on all objects of a given class without actually having to actively track anything.

When we moved to the heap implementation, this became a very difficult problem. And now to do an object release or mutation in the middle of the foreach loop, you need the recursive mutex.

Is there a way that we can remove the need to iterate over all pool objects in the first place? This would make things much, much simpler.

I would agree with that statement.

This locks the set (why a set?), then makes a copy of it, but keeps the original lock while iterating the copy. Why did we make the copy? Because a reentrant mutex doesn't make reentrant modification safe. Why hold the lock while working with a copy? There's no logic to this.

[kghost]

This locks the set (why a set?), then makes a copy of it, but keeps the original lock while iterating the copy. Why did we make the copy? Because a reentrant mutex doesn't make reentrant modification safe. Why hold the lock while working with a copy? There's no logic to this.

The iterator of std::set is not stable, if the handler deleted the object (which happens regularly when handling events), it wil crash in the middle of the loop, so I need to create a copy of the set.

I'll change it such that it only lock when creating the copy, and use normal mutex instead of a recursive mutex.

[msandstedt]

This locks the set (why a set?), then makes a copy of it, but keeps the original lock while iterating the copy. Why did we make the copy? Because a reentrant mutex doesn't make reentrant modification safe. Why hold the lock while working with a copy? There's no logic to this.

The iterator of std::set is not stable, if the handler deleted the object (which happens regularly when handling events), it wil crash in the middle of the loop, so I need to create a copy of the set.

I'll change it such that it only lock when creating the copy, and use normal mutex instead of a recursive mutex.

The copy becomes invalid as soon as the mutex is released.

[mspang]

This locks the set (why a set?), then makes a copy of it, but keeps the original lock while iterating the copy. Why did we make the copy? Because a reentrant mutex doesn't make reentrant modification safe. Why hold the lock while working with a copy? There's no logic to this.

The iterator of std::set is not stable, if the handler deleted the object (which happens regularly when handling events), it wil crash in the middle of the loop, so I need to create a copy of the set.
I'll change it such that it only lock when creating the copy, and use normal mutex instead of a recursive mutex.

The copy becomes invalid as soon as the mutex is released.

This locks the set (why a set?), then makes a copy of it, but keeps the original lock while iterating the copy. Why did we make the copy? Because a reentrant mutex doesn't make reentrant modification safe. Why hold the lock while working with a copy? There's no logic to this.

The iterator of std::set is not stable, if the handler deleted the object (which happens regularly when handling events), it wil crash in the middle of the loop, so I need to create a copy of the set.

I'll change it such that it only lock when creating the copy, and use normal mutex instead of a recursive mutex.

I'm not sure it's worth writing that up until we have better design consensus.

[yufengwangca]

Could we refactor code to clean out "#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP" in SystemPoolHeap.h ? since this file only contains heap based implementation. The #if check should be made in SystemPool.h

[kghost]

I'd prefer this way rather that

#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
#include <Impl_A>
#else
#include <Impl_B>
#endif

The proposing PR is more robust, because people may mistakenly include wrong headers, compiling wrong code, Which may hard to troubleshot.

[yufengwangca]

This file suppose to only implement non heap logic, CHIP_SYSTEM_CONFIG_POOL_USE_HEAP is not needed

[yufengwangca]

Please don't use recursive mutexes.

@mspang ,
The recursive mutex comes about with the original object pool design which was more than an allocator: it also allowed the sdk to iterate over all allocated objects in the pool. So instead of properly tracking state, we shoe-horned this iteration facility into an allocator to allow the sdk to do a foreach on all objects of a given class without actually having to actively track anything.
When we moved to the heap implementation, this became a very difficult problem. And now to do an object release or mutation in the middle of the foreach loop, you need the recursive mutex.
Is there a way that we can remove the need to iterate over all pool objects in the first place? This would make things much, much simpler.

I would agree with that statement.

This locks the set (why a set?), then makes a copy of it, but keeps the original lock while iterating the copy. Why did we make the copy? Because a reentrant mutex doesn't make reentrant modification safe. Why hold the lock while working with a copy? There's no logic to this.

No, the current implementation use std::mutex, it is not the original System Pool design, we should change back to std::mutex

[mrjerryjohns]

Why std::set? Seems like a std::list would be a more appropriate type here.

[kghost]

It would be very hard to implement ReleaseObject if std::list is used here. If using a intrusive_list, It will requires that the object is derived from a base class of intrusive_list<T>, We may not want to have this limitation.

[mrjerryjohns]

Instead of making this a recursive mutex, an alternative would be to alter the signature of Function to provide an inout arg that indicates if the item should be deleted. This then allows for clamping down what Function can do (which is not invoke ReleaseObject or CreateObject calls), and consequently, avoiding the need to make this a recursive mutex and still safely delete the list item while iterating.

This also avoids the need to make a copy of the set.

[mspang]

@yufengwangca

This PR contains all the changes of #9590, should we close #9590 and focus on this one instead?

@mspang

Can you please explain your changes?

Due to limitation of github, I can let this PR depend on other PRs, so I have to include all contents of related PRs into this one. We can either merge them one by one, or merge this PR and close others.

SystemLayer::PostEvent add API to post a lambda event #9678
Unify SystemPool and lib/support/Pool #9590
Merge SystemPool and support/Pool #9748

For those, a link in the description is sufficient not a copy of the whole text. But it is definitely a pain that this works so poorly in GitHub.

[yufengwangca]

DeferredRelease needs to be used for all Lwip implementations, we should move this back to SystemPool.

[kghost]

Why ? We will have lots of potential usages which do not need DeferredFree, Only TCPEndpoint and UDPEndpoint need it.

[yufengwangca]

Because DeferredRelease is a variant of Release in case the owner want to keep the object after release until the end of current event loop. This is not TCPEndPoint, UDPEndpoint, EndPointBasis specific. Otherwise, each object need to use this feature will have to implement a copy of this function by itself in future.

[kghost]

No, think it another way, it doesn't defer the release of the object, it is deferring the Release calls.

[yufengwangca]

What is the benefit to refactor this function out of SystemPool? What gains we get if each user of this feature need to implement a copy of this feature by itself.

[yufengwangca]

DeferredRelease needs to be used for all Lwip implementations, we should move this back to SystemPool.

[github-actions]

Size increase report for "gn_qpg-example-build" from 3647c29

File	Section	File	VM
chip-qpg6100-lighting-example.out	.bss	0	40
chip-qpg6100-lighting-example.out	.data	8	8
chip-qpg6100-lighting-example.out	.heap	0	-48
chip-qpg6100-lighting-example.out	.text	-136	-136

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_str,0,4446
.debug_ranges,0,304
.strtab,0,140
[Unmapped],0,136
.symtab,0,112
.bss,40,0
.data,8,8
.debug_aranges,0,-24
.heap,-48,0
.debug_frame,0,-52
.text,-136,-136
.debug_loc,0,-466
.debug_abbrev,0,-9258
.debug_line,0,-10755
.debug_info,0,-221039

[github-actions]

Size increase report for "esp32-example-build" from 3647c29

File	Section	File	VM
chip-all-clusters-app.elf	.dram0.bss	0	672
chip-all-clusters-app.elf	.iram0.text_end	0	56
chip-all-clusters-app.elf	.flash.text	52	52
chip-all-clusters-app.elf	.iram0.text	-56	-56
chip-all-clusters-app.elf	.flash.rodata	-72	-72

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,2741883
.debug_line,0,21538
.debug_abbrev,0,7154
.debug_str,0,5492
.dram0.bss,672,0
.strtab,0,229
.debug_frame,0,136
[Unmapped],0,76
.iram0.text_end,56,0
.flash.text,52,52
.debug_aranges,0,32
.symtab,0,16
.riscv.attributes,0,2
.shstrtab,0,-1
.iram0.text,-56,-56
.flash.rodata,-72,-72
.debug_loc,0,-133
.debug_ranges,0,-136

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.