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

Static / dynamic allocator #9922

Closed
wants to merge 2 commits into from
Closed

Conversation

cecille
Copy link
Contributor

@cecille cecille commented Sep 23, 2021

Problem

This is just a draft - wanted to get some feedback.

We have the System the SystemObjectPool to do pool based allocations now, but I was also working on a similar pool allocator for the mdns stuff and I wanted to get some feedback on incorporating this style of pool into the SystemObjectPool and whether that would mess anything up for you.

Desirable properties of the GenericAllocator

  • Can pass GenericAllocatorBase pointer to a function that isn't templated on the pool size (see second commit for example)
  • Uses the iterator style that's similar to the std implementations - this means we can use range-based for loops over the allocator which can make the code a bit more readable and works well with the current minimal mdns style
  • Can opt to use heap or static pool for each allocator - ie, can have a mix of heap and static as required.

Drawbacks

  • iterator style means we don't have the lock on the data like in the Funtion style of the SystemObjectPool. I think this is reasonable - it's fairly standard knowledge to not blindly manipulate an object while iterating through. This iterator usage matches what happens for std::list et. al.
  • SystemObjectPool does stats tracking that isn't implemented yet in GenericAllocator

So Question - would you be open to the idea of incorporating some of this functionality into the SystemObjectPool, such that we could have a SystemObjectPoolBase, an iterator accessor and per-pool heap/static configurability?

Change overview

Adds an Example for a static/dynamic allocator with tests, and shows how the allocator / iterator and base pointer would be use in the mdns layer.

Testing

  • unit tests added, mdns covered by minimal mdns unit tests for query responder and TestAdvertiser. Also confirmed mdns still working on lighting app in linux.

Allocator that can be set up at compile time to either allocate
from a static pool, or to use dynamic allocation.
@boring-cyborg boring-cyborg bot added the lib label Sep 23, 2021
@cecille cecille marked this pull request as draft September 23, 2021 14:41
@yufengwangca
Copy link
Contributor

We already have more than one pool implementation in our current system, #9748 is the last PR to converge them. The final gaol is we only have one pool implementation in CHIP SDK so the developers don't have to make decision which one to choose.

I found different pool implementations provide similar APIs, if there is some features not available on the current pool implementation, I would prefer enhance on top of it instead of creating a new one.

@cecille
Copy link
Contributor Author

cecille commented Sep 24, 2021

That sounds great - would you mind if I went in and made changes to the system pool to support an unsized base and an iterator? What are your feelings on the per-pool selection of static / dynamic? (templating change with 0 pool size for dynamic)

@yufengwangca
Copy link
Contributor

That sounds great - would you mind if I went in and made changes to the system pool to support an unsized base and an iterator? What are your feelings on the per-pool selection of static / dynamic? (templating change with 0 pool size for dynamic)

Please go ahead, we need to add the improvement on SystemPool for the feature we needed

@mrjerryjohns
Copy link
Contributor

mrjerryjohns commented Sep 29, 2021

I very much like the iterator pattern. I think that should replace the existing ForEachActiveObject method since it provides better flexibility in terms of logic you want to run across each item.

There is the question of locking which exists in the ForEachActiveObject method today, but I think that with the iterator pattern, that will have to move to the actual consumer interacting with that object to provide a similar synchronization guarantee.

@stale
Copy link

stale bot commented Oct 6, 2021

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 stale bot added the stale Stale issue or PR label Oct 6, 2021
@stale
Copy link

stale bot commented Oct 13, 2021

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

@stale stale bot closed this Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib stale Stale issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants