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

Add support for in-place instantiation of a shared message in the message pool #854

Conversation

SanderSmeenkInspiro
Copy link
Contributor

@SanderSmeenkInspiro SanderSmeenkInspiro commented Mar 3, 2024

First of all, thank you so much for writing the ETL!

With the current implementation of the reference counted message pool and the shared message, the message must first be created (e.g. on the stack) and is then copied into the message pool. However, sometimes a message needs to be instantiated in the message pool that is expensive to copy on an embedded platform.

If the message could be instantiated in-place in the memory pool (without a copy), this could save significant memory for certain use-cases.

This pull request is an example of what an implementation of in-place instantiation could look like.
The test suite is extended to demonstrate the usage ( message_pool.create_message<Message1>(1); )
In the test suite, the data element in the message is an int, but you can imagine this to be a type that is more expensive to copy.

Note: allocate(const TMessage*, Args&&... args) passes a pointer to TMessage, but it is only used to pass the type.
There is undoubtedly a better way to achieve this, that I have not found to work yet.

Feel free to comment or request changes, I'd be happy to incorporate those.

Copy link

semanticdiff-com bot commented Mar 3, 2024

Review changes with SemanticDiff.

@SanderSmeenkInspiro SanderSmeenkInspiro force-pushed the feature/in-place-construction-of-shared-message branch from 0d02eca to 8d21338 Compare March 3, 2024 19:48
@SanderSmeenkInspiro SanderSmeenkInspiro force-pushed the feature/in-place-construction-of-shared-message branch from 8d21338 to a196b48 Compare March 3, 2024 19:50
@jwellbelove jwellbelove changed the base branch from master to pull-request/#854-in-place-construction-of-shared-message March 8, 2024 09:48
…-message' into feature/in-place-construction-of-shared-message
@jwellbelove
Copy link
Contributor

I looked at the code and I've experimentally modified the code.

I added a utility class etl::type_tag<T> that can replace the dummy pointer.

//*************************************************************************
/// Creator for in-place instantiation
//*************************************************************************
template <typename TMessage, typename TPool, typename... Args>
static shared_message create(TPool& owner, Args&&... args)
{
  return shared_message(owner, etl::type_tag<TMessage>(), etl::forward<Args>(args)...);
}
    
//*************************************************************************
/// Constructor
//*************************************************************************
template <typename TPool, typename TMessage, typename... TArgs>
shared_message(TPool& owner, etl::type_tag<TMessage>, TArgs&&... args)
{
  ETL_STATIC_ASSERT((etl::is_base_of<etl::ireference_counted_message_pool, TPool>::value), "TPool not derived from etl::ireference_counted_message_pool");
  ETL_STATIC_ASSERT((etl::is_base_of<etl::imessage, TMessage>::value), "TMessage not derived from etl::imessage");

  p_rcmessage = owner.template allocate<TMessage>(etl::forward<TArgs>(args)...);

  if (p_rcmessage != ETL_NULLPTR)
  {
    p_rcmessage->get_reference_counter().set_reference_count(1U);
  }
}

You'll notice that the emplace allocate function now takes the message type as a template parameter, rather than as a dummy pointer argument.

I'll merge your code and add my changes.

@jwellbelove jwellbelove merged commit 268ca4e into ETLCPP:pull-request/#854-in-place-construction-of-shared-message Mar 8, 2024
32 of 59 checks passed
jwellbelove pushed a commit that referenced this pull request Mar 9, 2024
jwellbelove pushed a commit that referenced this pull request Mar 9, 2024
@SanderSmeenkInspiro
Copy link
Contributor Author

Great!

@jwellbelove
Copy link
Contributor

I removed the etl::type_tag<T> and used the already existing STL compatible class etl::in_place_type_t<T>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants