make MemoryPool compatible with classes

[JojoS62]

Summary of changes

Fix for #14503

When MemoryPool type T is non-trivial (class, struct) with constructor, the constructor of the element is not called and will give unpredicted results when used after mempool.alloc().
Using MemoryPool with classes gives no warning/error, the code compiles but will produce runtime errors.

With this PR, each elements constructor is called when is_trivial is false.
Important: the memory pool management is done by CMSIS/RTX osMemoryPool functions. The MemoryPool constructor calls osMemoryPoolAlloc(), which initializes the raw memory block with pointers at each element to point to the next free block. These pointers must not be touched and would collide with the class memory space. Therefore, the sizeof(T) is increased by the sizeof one pointer and the class memory is starting after this pointer. The alloc/free functions must take this into account and adust the outgoing/incoming pointers.

Impact of changes

In case of a trivial type like a simple struct, the requirered size is also n * (sizeof(T) + 4). so four bytes per element more than before. The pool memory is a buffer as member var of MemoryPool and may reside on the stack. My suggestion is to move this block to the heap. In this case, the optional 4 bytes per element can be saved because at runtime, the necessary size can be calculated.
Another feature enhancement would be an external pointer to the raw memory, supplied in the constructor of MemoryPool. That is usefull for MCUs with scattered memory, then the MemoryPool could manage this partitial memory.

A decision has to made for the usage of calloc(): this is usually not allowed when T is of type class. It can be ok when all members are filled with 0, but specially when a member is a pointer to some additional memory, using calloc() will destroy these pointers.

Migration actions required

Documentation

A note about using MemoryPool with classes should be added. It is important that objects in the pool are only constructed once, the alloc() function can deliver 'used' objects with random state.
A use case for MemoryPool with classes is e.g.:

Mail<MIDIMessage, 32> mailboxMIDIMessages;

The MIDIMessage uses additional space from the heap, initialized in the constructor. With this PR, the handling of MIDIMessages also in interrupt context is save because the necessary memory is allocted in advance.

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

The MemoryPool tests has passed. But this test uses struct as type T, tests with class type need to be added.
This is also important as the modification relies on internal behaviour of osMemoryPool.

---

Reviewers

---

[ciarmcom]

@JojoS62, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

The semantics here are rather unusual - I would expect an allocator by default to be constructing and destroying the objects in the alloc and free functions, not to be giving you a pool of preconstructed objects.

I do see the potential use-case, in particular for the interrupt-safe taking of a descriptor, but it feels a bit of an API stretch. I guess you do get more than you would be just having your own "free list" - you can leverage off of the "blocking" facility here. Although you could get those same semantics for a manual free list via a ConditionVariable signalling the free list head, or a Semaphore counting its entries.

You're also relying on the implementation of Memory Pools - you're assuming that its free then alloc only touches the first word, rather than scrubbing the memory contents. I think that pushes it over the line of "not viable" for me. I would class the construction and destruction of non-allocated blocks in MemoryPool() and ~MemoryPool() and the assumption that newly-allocated blocks still have a constructed object as "undefined behaviour".

CMSIS-RTOS 2 docs say "One must not write to freed block. It is up to the implementation to reuse the memory of unused blocks for internal control data, i.e. linked list pointers."

I'd want to see this support "construct on alloc", and if it weren't for that implementation dependence problem it could support "construct on init" (switched via a template parameter).

But the default behaviour for backwards compatibility would have to remain "no construct". Users may already be using MemoryPool with non-trivial classes, relying on it not doing construction, and doing their own construction/deconstruction, and this patch would break them.

I've chimed in on previous discussions about this in the past - you're not the first to look at this.

#10664
#9435

[JojoS62]

The actual behaviour is, that you get no warning about using a class and memorypool.alloc() wil hand out uninitialized objects. When I'm not the first one who spent time in debugging, it looks like this behaviour is not nice.
The problem happens when using Mail and the alloc is called in ISR, then it is not allowed to call new (which exists e.g. in the MIDIMessage or other constructors). For this use case, you do not want to do expensive initialization in the ISR and the Mail/MemoryPool with initialized objects would be a simple solution.
Using an own free block management will cost more memory, the implicit management in the pool memory is more efficient.

How about adding a 'doInitialize' switch to the constructor of the MemoryPool for the backwards compatibility? Default can be false for compatible behaviour.
I'm aware about the 'stretching' of the RTOS API, but the expected behaviour of the alloc/free treatment of memory blocks can be checked with tests. And is it really probably that the implementation changes? The used linked list is simple and fast, why should that change?

[kjbracey]

The actual behaviour is, that you get no warning about using a class and memorypool.alloc() wil hand out uninitialized objects. When I'm not the first one who spent time in debugging, it looks like this behaviour is not nice.

It's potentially unexpected, yes. But on the other hand as I mentioned in one of the previous discussions, it's justifiable. The standard C++ Allocator will return an uninitialised T *.

The problem happens when using Mail and the alloc is called in ISR, then it is not allowed to call new (which exists e.g. in the MIDIMessage or other constructors). For this use case, you do not want to do expensive initialization in the ISR and the Mail/MemoryPool with initialized objects would be a simple solution.

Yes, I certainly see the use case.

How about adding a 'doInitialize' switch to the constructor of the MemoryPool? Default can be false for compatible behaviour.

I would do it as a template parameter. You're using templates anyway, and I can't imagine there being two users of MemoryPool<T> in the system wanting different allocation behaviour, so it would be most efficient to do it as compile-time selection to lock in the choice, rather than run-time code and storage of mode.

Plus there would be the storage issue - the other modes (construct on alloc, or no construct) wouldn't need the extra "avoid the pointer" allocation overhead you've put in unconditionally. Template parameter would let that be conditional.

I'm aware about the 'stretching' of the RTOS API, but the expected behaviour of the alloc/free treatment of memory blocks can be checked with tests. And is it really probably that the implementation changes? The used linked list is simple and fast, why should that change?

Things do change. For allocators deliberate guard word protection to catch accessing of freed blocks as a debug tool is certainly a thing that happens.

There is a problem here in that Mbed OS APIs are attempting to be stable - possibly more than CMSIS-RTOS. We've already covered up the CMSIS-RTOS 1->2 API incompatibilities, and that was a pain. To add an Mbed OS API whose implementation relies on an implementation detail of the current RTX potentially leaves us exposed if that changes. (And there are already unofficial Mbed OS ports to non-RTX systems).

I'm not 100% opposed to adding "pre-construct" (and it's not my call), but I'd want to see it stuck under something like the "experimental" gating flag that was recently done for virtualised HAL objects, so we're not committed to maintenance.

[JojoS62]

I would do it as a template parameter.

I have no clue how this could look like, an additional template parameter to the existing type T?

I'm not 100% opposed to adding "pre-construct" (and it's not my call), but I'd want to see it stuck under something like the "experimental" gating

these configuration variants are already hard stuff, and make it very hard to write libraries that work for more than one special mbed version. There are already too many deprecation changes, even the MemoryPool has more deprecated than current members.

The Mail/MemoryPool classes seemed to fit well for a fast data/object transport from ISR to thread. An alternative solution would be a circular buffer as a simplifed MemoryPool. There is a CircularBuffer class in the Mbed API already, but it has no event flags for blocking wait. So it maybe a better solution to create this combination? Or does it exist already?

[kjbracey]

I would do it as a template parameter.

I have no clue how this could look like, an additional template parameter to the existing type T?

Add a template parameter with an enum for the modes, and giving it a default.

MemoryPool<class T, uint32_t pool_sz, int mode = MemoryPoolAllocType::no_construct>

these configuration variants are already hard stuff, and make it very hard to write libraries that work for more than one special mbed version. There are already too many deprecation changes, even the MemoryPool has more deprecated than current members.

That's also an argument for not extending APIs - make it work for old versions :)

The Mail/MemoryPool classes seemed to fit well for a fast data/object transport from ISR to thread. An alternative solution would be a circular buffer as a simplifed MemoryPool. There is a CircularBuffer class in the Mbed API already, but it has no event flags for blocking wait. So it maybe a better solution to create this combination? Or does it exist already?

Well, look at Mail which uses the combination of a Queue and a MemoryPool. It puts pointers to the blocks into the Queue. But in your use case you're never actually allocating or freeing the memory, it's permanently reserved, so for you a MemoryPool isn't buying you anything, really.

You could make an analogue to Mail that instead had static storage - an array of T, and 2 Queues of pointers into that array, one for in-use and one for free. Total overhead would be the same - the extra Queue of N T * for the free list taking the place of the allocation overhead in your MemoryPool.

(Note that Mail is convoluted due to it being CMSIS-RTOS 1 era. In CMSIS-RTOS 2 you could implement Mail by just putting your Ts into the CMSIS-RTOS 2 Queue, but CMSIS-RTOS 1 only supported pointer-sized elements in a Queue. Mbed OS's Queue retains this CMSIS-RTOS 1 API, and has not been extended to support anything other than pointers. But this matches your needs precisely).

[JojoS62]

You could make an analogue to Mail that instead had static storage - an array of T, and 2 Queues of pointers into that array, one for in-use and one for free. Total overhead would be the same - the extra Queue of N T * for the free list taking the place of the allocation overhead in your MemoryPool.

That sounds like a good solution. The RTOS 2 Queue may store T, but doesn't have the read and write pointers like a circular buffer. Two queues will waste also a little more memory, because you need only a read/write index into a pointer list. But the 2 Queues solution may benefit from the existing thread safety?

so for you a MemoryPool isn't buying you anything, really.

it handles the used / free management and is fast, without searching for a free block.

[kjbracey]

That sounds like a good solution. The RTOS 2 Queue may store T, but doesn't have the read and write pointers like a circular buffer. Two queues will waste also a little more memory, because you need only a read/write index into a pointer list. But the 2 Queues solution may benefit from the existing thread safety?

If your application is purely circular, release in order of alloc, then the MemoryPool was already overkill. My Queue suggestion was retaining the same arbitrary-release-order ability.

Something similar to CircularBuffer would be most memory-efficient, but you'd need to do more fiddling - can't just leverage the existing class. (And that class doesn't have blocking support.)

Queue should have all the necessary thread-safety you need, including the ability to block while waiting for queue space if necessary.

it handles the used / free management and is fast, without searching for a free block.

Indeed, but you're having to stretch its storage with the void * overhead. You've effectively added a Queue's worth of storage into it manually, and you're treating it as if it was a Queue plus an array. Better to do that explicitly.

[JojoS62]

Queue should have all the necessary thread-safety you need, including the ability to block while waiting for queue space if necessary.

I agree and will try this. More difficult: what name shoud it have? MailQueue?

[JojoS62]

Thanks Kevin for your comments and suggestions. The replacement by using an Array and two queues was simple and looks much cleaner. I think I'm understanding now also the discussion about the naming and meaning of the 'alloc' member. Anyway, the behaviour that objects are not constructed, should be mentionend in the documentation. Even if there is a semantic difference between allocating and creating an object.

So I close this PR and will make a proposal for a new 'ObjectMailbox' with the functionality of handing out and storing objects in FIFO order.

[kjbracey]

Anyway, the behaviour that objects are not constructed, should be mentionend in the documentation.

Strongly agree, if it isn't already mentioned. PRs to add to the doxygen also welcome.

[0xc0170]

Strongly agree, if it isn't already mentioned. PRs to add to the doxygen also welcome.

Can you create a fix to the doxygen doc please?

[kjbracey]

Opened #14537 for doxygen change.