Skip to content

feat: ability to override the default allocator #508

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.

Sign up for GitHub

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

Open
russkel wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

feat: ability to override the default allocator #508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions include/rcutils/allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,21 @@ RCUTILS_WARN_UNUSED
rcutils_allocator_t
rcutils_get_default_allocator(void);

/// Override the default allocator returned by rcutils_get_default_allocator.
/**
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
Comment on lines +116 to +118
Copy link
Member

@wjwwood wjwwood Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about having this be non-thread-safe, because it's unlikely that calling code will have the ability to synchronize with other calling threads and/or with other threads reading the value. My guess is that for this to be useful, it needs to be thread-safe and that to do so, using atomics and no locks (assuming the atomics are lock-free) would be ideal.

*
* \param[in] override_allocator The allocator to set as the default.
*/
RCUTILS_PUBLIC
void
rcutils_set_default_allocator(rcutils_allocator_t override_allocator);

/// Return true if the given allocator has non-null function pointers.
/**
* \param[in] allocator to be checked by the function
Expand Down
15 changes: 15 additions & 0 deletions src/allocator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,31 @@ rcutils_get_zero_initialized_allocator(void)
return zero_allocator;
}

static rcutils_allocator_t rcutils_override_default_allocator = {0};

void
rcutils_set_default_allocator(rcutils_allocator_t override_allocator)
{
if (rcutils_allocator_is_valid(&override_allocator)) {
Copy link
Member

@wjwwood wjwwood Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this check on the setter, is it impossible to reset the default allocator to be the "real default"? I would imagine, you could do something like rcutils_set_default_allocator({0});, but not with this check. Maybe it should just assign the allocator value no matter what?

rcutils_override_default_allocator = override_allocator;
}
}

rcutils_allocator_t
rcutils_get_default_allocator(void)
{
if (rcutils_allocator_is_valid(&rcutils_override_default_allocator)) {
return rcutils_override_default_allocator;
}
Comment on lines +85 to +87
Copy link
Member

@wjwwood wjwwood Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three concerns here:

  • Thread-safety: what happens if some other thread calls rcutils_set_default_allocator() between lines 85 and 85? Not to mention line 86 being run in the middle of the assignment that happens on line 78.
  • Performance: this check will be called potentially many, many times, even in critical code sections, and having a branch here might be expensive. It would be great to either eliminate the check, or use some kind of branch prediction hints, like __builtin_expect or something similar. I believe we use this kind of thing in the console logging code in this repository.
  • Is this redundant? calling code is likely to check that the allocator is valid and right now the setter for this first checks that the allocator is valid, so this check is probably redundant (unless we remove the check in the setter).


static rcutils_allocator_t default_allocator = {
.allocate = __default_allocate,
.deallocate = __default_deallocate,
.reallocate = __default_reallocate,
.zero_allocate = __default_zero_allocate,
.state = NULL,
};

return default_allocator;
}

Expand Down