Skip to content

Commit

Permalink
Swap to using size_t as an id type (#92)
Browse files Browse the repository at this point in the history
uint64_t is larger than is necessary on a system for IDs most of the
time.
On 32-bit systems, it also causes problems due to uint64_t having
alignment requirements.
Swaps to using size_t for the ID which should serve the purposes of
being fast and appropriately sized for the system.
  • Loading branch information
TrentHouliston authored Sep 25, 2023
1 parent 2fbf7e3 commit 6480a09
Show file tree
Hide file tree
Showing 16 changed files with 94 additions and 43 deletions.
2 changes: 1 addition & 1 deletion src/PowerPlant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ void PowerPlant::start() {
scheduler.start();
}

void PowerPlant::submit(const uint64_t& id,
void PowerPlant::submit(const NUClear::id_t& id,
const int& priority,
const util::GroupDescriptor& group,
const util::ThreadPoolDescriptor& pool,
Expand Down
3 changes: 2 additions & 1 deletion src/PowerPlant.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
// Utilities
#include "Configuration.hpp"
#include "LogLevel.hpp"
#include "id.hpp"
#include "message/LogMessage.hpp"
#include "threading/ReactionTask.hpp"
#include "threading/TaskScheduler.hpp"
Expand Down Expand Up @@ -135,7 +136,7 @@ class PowerPlant {
* @param immediate if this task should run immediately in the current thread
* @param task the wrapped function to be executed
*/
void submit(const uint64_t& id,
void submit(const NUClear::id_t& id,
const int& priority,
const util::GroupDescriptor& group,
const util::ThreadPoolDescriptor& pool,
Expand Down
9 changes: 5 additions & 4 deletions src/dsl/operation/ChronoTask.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#define NUCLEAR_DSL_OPERATION_CHRONOTASK_HPP

#include "../../clock.hpp"
#include "../../id.hpp"

namespace NUClear {
namespace dsl {
Expand All @@ -46,11 +47,11 @@ namespace dsl {
* @param task the task to run, takes the time to execute as a reference so it can be updated for
* future runs
* @param time the time to execute this task
* @param id the unique identifer for this task
* @param id the unique identifier for this task
*/
ChronoTask(std::function<bool(NUClear::clock::time_point&)>&& task,
NUClear::clock::time_point time,
uint64_t id)
const NUClear::clock::time_point& time,
const NUClear::id_t& id)
: task(std::move(task)), time(time), id(id) {}

/**
Expand Down Expand Up @@ -100,7 +101,7 @@ namespace dsl {
/// The time this task should be executed
NUClear::clock::time_point time;
/// The unique identifier for this task so it can be unbound
uint64_t id{0};
NUClear::id_t id{0};
};

} // namespace operation
Expand Down
6 changes: 4 additions & 2 deletions src/dsl/operation/Unbind.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#ifndef NUCLEAR_DSL_OPERATION_UNBIND_HPP
#define NUCLEAR_DSL_OPERATION_UNBIND_HPP

#include "../../id.hpp"

namespace NUClear {
namespace dsl {
namespace operation {
Expand All @@ -38,10 +40,10 @@ namespace dsl {
*/
template <typename Word>
struct Unbind {
explicit Unbind(const uint64_t& id) : id(id){};
explicit Unbind(const NUClear::id_t& id) : id(id){};

/// The id of the task to unbind
const uint64_t id{0};
const NUClear::id_t id{0};
};

} // namespace operation
Expand Down
5 changes: 3 additions & 2 deletions src/dsl/word/Always.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <mutex>
#include <utility>

#include "../../id.hpp"
#include "../../threading/ReactionTask.hpp"
#include "../../util/ThreadPoolDescriptor.hpp"

Expand Down Expand Up @@ -73,7 +74,7 @@ namespace dsl {

template <typename DSL>
static inline util::ThreadPoolDescriptor pool(const threading::Reaction& reaction) {
static std::map<uint64_t, uint64_t> pool_id;
static std::map<NUClear::id_t, NUClear::id_t> pool_id;
static std::mutex mutex;

const std::lock_guard<std::mutex> lock(mutex);
Expand All @@ -90,7 +91,7 @@ namespace dsl {
* the always reaction and one for the idle reaction that we generate in this function
* The main purpose of this map is to ensure that the always reaction pointer doesn't get destroyed
*/
static std::map<uint64_t,
static std::map<NUClear::id_t,
std::pair<std::shared_ptr<threading::Reaction>, std::shared_ptr<threading::Reaction>>>
reaction_store = {};

Expand Down
5 changes: 3 additions & 2 deletions src/dsl/word/IO.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#ifndef NUCLEAR_DSL_WORD_IO_HPP
#define NUCLEAR_DSL_WORD_IO_HPP

#include "../../id.hpp"
#include "../../threading/Reaction.hpp"
#include "../../util/platform.hpp"
#include "../operation/Unbind.hpp"
Expand Down Expand Up @@ -59,9 +60,9 @@ namespace dsl {
* @brief This is emitted when an IO operation has finished.
*/
struct IOFinished {
explicit IOFinished(const uint64_t& id) : id(id) {}
explicit IOFinished(const NUClear::id_t& id) : id(id) {}
/// @brief The id of the reaction that has finished
uint64_t id;
NUClear::id_t id;
};

/**
Expand Down
37 changes: 37 additions & 0 deletions src/id.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* MIT License
*
* Copyright (c) 2023 NUClear Contributors
*
* This file is part of the NUClear codebase.
* See https://github.com/Fastcode/NUClear for further info.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
* documentation files (the "Software"), to deal in the Software without restriction, including without limitation the
* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to
* permit persons to whom the Software is furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all copies or substantial portions of the
* Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
* WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/

#ifndef NUCLEAR_UTIL_ID_HPP
#define NUCLEAR_UTIL_ID_HPP

#include <cstdint>

namespace NUClear {

/**
* @brief A unique identifier for a thread pool
*/
using id_t = std::size_t;

} // namespace NUClear

#endif // NUCLEAR_UTIL_ID_HPP
17 changes: 9 additions & 8 deletions src/message/ReactionStatistics.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <exception>
#include <string>
#include <vector>
#include "../id.hpp"

#include "../clock.hpp"
#include "../threading/ReactionIdentifiers.hpp"
Expand All @@ -39,10 +40,10 @@ namespace message {
struct ReactionStatistics {

ReactionStatistics(threading::ReactionIdentifiers identifiers,
uint64_t reaction_id,
uint64_t task_id,
uint64_t cause_reaction_id,
uint64_t cause_task_id,
const NUClear::id_t& reaction_id,
const NUClear::id_t& task_id,
const NUClear::id_t& cause_reaction_id,
const NUClear::id_t& cause_task_id,
const clock::time_point& emitted,
const clock::time_point& start,
const clock::time_point& finish,
Expand All @@ -60,13 +61,13 @@ namespace message {
/// @brief A string containing the username/on arguments/and callback name of the reaction.
threading::ReactionIdentifiers identifiers;
/// @brief The id of this reaction.
uint64_t reaction_id{0};
NUClear::id_t reaction_id{0};
/// @brief The task id of this reaction.
uint64_t task_id{0};
NUClear::id_t task_id{0};
/// @brief The reaction id of the reaction that caused this one or 0 if there was not one
uint64_t cause_reaction_id{0};
NUClear::id_t cause_reaction_id{0};
/// @brief The reaction id of the task that caused this task or 0 if there was not one
uint64_t cause_task_id{0};
NUClear::id_t cause_task_id{0};
/// @brief The time that this reaction was emitted to the thread pool
clock::time_point emitted{};
/// @brief The time that execution started on this reaction
Expand Down
2 changes: 1 addition & 1 deletion src/threading/Reaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace NUClear {
namespace threading {

// Initialize our reaction source
std::atomic<uint64_t> Reaction::reaction_id_source(0); // NOLINT
std::atomic<NUClear::id_t> Reaction::reaction_id_source(0); // NOLINT

Reaction::Reaction(Reactor& reactor, ReactionIdentifiers&& identifiers, TaskGenerator&& generator)
: reactor(reactor), identifiers(std::move(identifiers)), generator(std::move(generator)) {}
Expand Down
6 changes: 4 additions & 2 deletions src/threading/Reaction.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <functional>
#include <memory>
#include <string>
#include "../id.hpp"
#include <utility>

#include "../util/GeneratedCallback.hpp"
Expand Down Expand Up @@ -107,7 +108,7 @@ namespace threading {
ReactionIdentifiers identifiers;

/// @brief the unique identifier for this Reaction object
const uint64_t id{++reaction_id_source};
const NUClear::id_t id{++reaction_id_source};

/// @brief if this is false, we cannot emit ReactionStatistics from any reaction triggered by this one
bool emit_stats{true};
Expand All @@ -128,7 +129,8 @@ namespace threading {

private:
/// @brief a source for reaction_ids, atomically creates longs
static std::atomic<uint64_t> reaction_id_source; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
static std::atomic<NUClear::id_t>
reaction_id_source; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
/// @brief the callback generator function (creates databound callbacks)
TaskGenerator generator;
};
Expand Down
10 changes: 5 additions & 5 deletions src/threading/ReactionTask.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
#include <memory>
#include <typeindex>
#include <vector>

#include "../id.hpp"
#include "../message/ReactionStatistics.hpp"
#include "../util/GroupDescriptor.hpp"
#include "../util/ThreadPoolDescriptor.hpp"
Expand All @@ -50,7 +50,7 @@ namespace threading {
class Task {
private:
/// @brief a source for task ids, atomically creates longs
static std::atomic<std::uint64_t> task_id_source; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
static std::atomic<NUClear::id_t> task_id_source; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)

/// @brief the current task that is being executed by this thread (or nullptr if none is)
static ATTRIBUTE_TLS Task* current_task; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
Expand Down Expand Up @@ -121,14 +121,14 @@ namespace threading {
*
* @return a new unique task id
*/
static inline uint64_t new_task_id() {
static inline NUClear::id_t new_task_id() {
return ++task_id_source;
}

/// @brief the parent Reaction object which spawned this
ReactionType& parent;
/// @brief the task id of this task (the sequence number of this particular task)
uint64_t id{new_task_id()};
NUClear::id_t id{new_task_id()};
/// @brief the priority to run this task at
int priority;
/// @brief the statistics object that persists after this for information and debugging
Expand All @@ -151,7 +151,7 @@ namespace threading {

// Initialize our id source
template <typename ReactionType>
std::atomic<uint64_t> Task<ReactionType>::task_id_source(0); // NOLINT
std::atomic<NUClear::id_t> Task<ReactionType>::task_id_source(0); // NOLINT

// Initialize our current task
template <typename ReactionType>
Expand Down
2 changes: 1 addition & 1 deletion src/threading/TaskScheduler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ namespace threading {
}
}

void TaskScheduler::submit(const uint64_t& id,
void TaskScheduler::submit(const NUClear::id_t& id,
const int& priority,
const util::GroupDescriptor& group_descriptor,
const util::ThreadPoolDescriptor& pool_descriptor,
Expand Down
9 changes: 5 additions & 4 deletions src/threading/TaskScheduler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include <thread>
#include <vector>

#include "../id.hpp"
#include "../util/GroupDescriptor.hpp"
#include "../util/ThreadPoolDescriptor.hpp"
#include "../util/platform.hpp"
Expand Down Expand Up @@ -93,7 +94,7 @@ namespace threading {
*/
struct Task {
/// @brief The id of this task used for ordering
uint64_t id;
NUClear::id_t id;
/// @brief The priority of this task
int priority;
/// @brief The group descriptor for this task
Expand Down Expand Up @@ -168,7 +169,7 @@ namespace threading {
* as normal
* @param func the function to execute
*/
void submit(const uint64_t& id,
void submit(const NUClear::id_t& id,
const int& priority,
const util::GroupDescriptor& group,
const util::ThreadPoolDescriptor& pool,
Expand Down Expand Up @@ -242,12 +243,12 @@ namespace threading {
std::atomic<bool> started{false};

/// @brief A map of group ids to the number of active tasks currently running in that group
std::map<uint64_t, size_t> groups{};
std::map<NUClear::id_t, size_t> groups{};
/// @brief mutex for the group map
std::mutex group_mutex;

/// @brief A map of pool descriptor ids to pool descriptors
std::map<uint64_t, std::shared_ptr<PoolQueue>> pool_queues{};
std::map<NUClear::id_t, std::shared_ptr<PoolQueue>> pool_queues{};
/// @brief a mutex for when we are modifying the pool_queues map
std::mutex pool_mutex;
/// @brief a pointer to the pool_queue for the current thread so it does not have to access via the map
Expand Down
8 changes: 5 additions & 3 deletions src/util/GroupDescriptor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include <cstdint>
#include <limits>

#include "../id.hpp"

namespace NUClear {
namespace util {

Expand All @@ -36,17 +38,17 @@ namespace util {
*/
struct GroupDescriptor {
/// @brief a unique identifier for this pool
uint64_t group_id{0};
NUClear::id_t group_id{0};

/// @brief the maximum number of threads that can run concurrently in this group
size_t thread_count{std::numeric_limits<size_t>::max()};

/**
* @brief Return the next unique ID for a new group
*/
static uint64_t get_unique_group_id() noexcept {
static NUClear::id_t get_unique_group_id() noexcept {
// Make group 0 the default group
static std::atomic<uint64_t> source{1};
static std::atomic<NUClear::id_t> source{1};
return source++;
}
};
Expand Down
4 changes: 2 additions & 2 deletions src/util/ThreadPoolDescriptor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
namespace NUClear {
namespace util {

const uint64_t ThreadPoolDescriptor::MAIN_THREAD_POOL_ID = 0;
const uint64_t ThreadPoolDescriptor::DEFAULT_THREAD_POOL_ID = 1;
const NUClear::id_t ThreadPoolDescriptor::MAIN_THREAD_POOL_ID = 0;
const NUClear::id_t ThreadPoolDescriptor::DEFAULT_THREAD_POOL_ID = 1;

} // namespace util
} // namespace NUClear
Loading

0 comments on commit 6480a09

Please sign in to comment.