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

Avoid uint64_t operation when handling context #16

Merged
merged 5 commits into from
May 9, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions rcl/include/rcl/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,6 @@ typedef struct rcl_context_t
// The assumption that this is big enough for an atomic_uint_least64_t is
// ensured with a static_assert in the context.c file.
// In most cases it should just be a plain uint64_t.
/// @cond Doxygen_Suppress
#if !defined(RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE)
#define RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE sizeof(uint_least64_t)
#endif
/// @endcond
/// Private storage for instance ID atomic.
/**
* Accessing the instance id should be done using the function
Expand All @@ -150,7 +145,7 @@ typedef struct rcl_context_t
* See this paper for an effort to make this possible in the future:
* http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0943r1.html
*/
RCL_ALIGNAS(8) uint8_t instance_id_storage[RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE];
uint32_t instance_id_storage;
} rcl_context_t;

/// Return a zero initialization context object.
Expand Down
12 changes: 3 additions & 9 deletions rcl/src/rcl/context.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,12 @@ rcl_get_zero_initialized_context(void)
{
static rcl_context_t context = {
.impl = NULL,
.instance_id_storage = {0},
.instance_id_storage = 0,
};
// this is not constexpr so it cannot be in the struct initialization
#ifdef RCL_COMMAND_LINE_ENABLED
context.global_arguments = rcl_get_zero_initialized_arguments();
#endif // RCL_COMMAND_LINE_ENABLED
// ensure assumption about static storage
static_assert(
sizeof(context.instance_id_storage) >= sizeof(atomic_uint_least64_t),
"expected rcl_context_t's instance id storage to be >= size of atomic_uint_least64_t");
// initialize atomic
atomic_init((atomic_uint_least64_t *)(&context.instance_id_storage), 0);
return context;
}

Expand Down Expand Up @@ -78,7 +72,7 @@ rcl_context_instance_id_t
rcl_context_get_instance_id(const rcl_context_t * context)
{
RCL_CHECK_ARGUMENT_FOR_NULL(context, 0);
return rcutils_atomic_load_uint64_t((atomic_uint_least64_t *)(&context->instance_id_storage));
return context->instance_id_storage;
}

rcl_ret_t
Expand Down Expand Up @@ -112,7 +106,7 @@ __cleanup_context(rcl_context_t * context)
{
rcl_ret_t ret = RCL_RET_OK;
// reset the instance id to 0 to indicate "invalid" (should already be 0, but this is defensive)
rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0);
context->instance_id_storage = 0;

#ifdef RCL_COMMAND_LINE_ENABLED
// clean up global_arguments if initialized
Expand Down
20 changes: 10 additions & 10 deletions rcl/src/rcl/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ extern "C"
#include "./context_impl.h"
#include "./init_options_impl.h"

static atomic_uint_least64_t __rcl_next_unique_id = ATOMIC_VAR_INIT(1);

rcl_ret_t
rcl_init(
int argc,
Expand Down Expand Up @@ -142,15 +140,17 @@ rcl_init(
#endif // RCL_COMMAND_LINE_ENABLED

// Set the instance id.
uint64_t next_instance_id = rcutils_atomic_fetch_add_uint64_t(&__rcl_next_unique_id, 1);
static uint32_t next_instance_id = 0;
next_instance_id++;
if (0 == next_instance_id) {
// Roll over occurred, this is an extremely unlikely occurrence.
RCL_SET_ERROR_MSG("unique rcl instance ids exhausted");
// Roll back to try to avoid the next call succeeding, but there's a data race here.
rcutils_atomic_store(&__rcl_next_unique_id, -1);
goto fail;
next_instance_id++;
// // Roll over occurred, this is an extremely unlikely occurrence.
Acuadros95 marked this conversation as resolved.
Show resolved Hide resolved
// RCL_SET_ERROR_MSG("unique rcl instance ids exhausted");
// // Roll back to try to avoid the next call succeeding, but there's a data race here.
// rcutils_atomic_store(&__rcl_next_unique_id, -1);
// goto fail;
}
rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id);
context->instance_id_storage = next_instance_id;
context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id;

size_t * domain_id = &context->impl->init_options.impl->rmw_init_options.domain_id;
Expand Down Expand Up @@ -261,7 +261,7 @@ rcl_shutdown(rcl_context_t * context)
}

// reset the instance id to 0 to indicate "invalid"
rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0);
context->instance_id_storage = 0;

return RCL_RET_OK;
}
Expand Down