I aim to create a call queue to schedule work for another thread.
I'm using a virtual base class for type erasure and placement new for reducing the number of allocations, but, as far as I know, reinterpret_cast<Base*>(raw_ptr)
is UB.
Is there any other way to implement it?
class FuncWrapperInterface
{
public:
virtual ~FuncWrapperInterface() = default;
virtual void operator()() = 0;
virtual size_t size() = 0;
};
template<typename Func>
class FuncWrapper : public FuncWrapperInterface
{
public:
FuncWrapper(Func&& func)
: func_{std::move(func)}
{
}
void operator()() override
{
func_();
}
size_t size() override;
private:
Func func_;
};
template<typename Func>
size_t FuncWrapper<Func>::size()
{
return sizeof(*this);
}
class FuncQueue
{
public:
template<typename Func>
void add(Func&& func)
{
auto start{data_.size()};
data_.resize(data_.size() + sizeof(FuncWrapper<Func>));
new (data_.data() + start) FuncWrapper<Func>{std::move(func)};
}
void execute()
{
for (size_t idx{0}; idx < data_.size();)
{
// problem here
auto fw{std::launder(reinterpret_cast<FuncWrapperInterface*>(data_.data() + idx))};
(*fw)();
idx += fw->size();
fw->~FuncWrapperInterface();
}
data_.clear();
}
private:
std::vector<std::byte> data_;
};
UPD: I assume storing a pointer to FuncWrapperInterface
will solve that problem. Only alignment and exception/destruction issues remain.
Correct me if I am wrong.
minimal example:
int main()
{
FuncQueue fq;
fq.add([&]() { std::cout << "3" << std::endl; });
fq.add([&]() { std::cout << "2" << std::endl; });
fq.add([&]() { std::cout << "1" << std::endl; });
fq.add([&]() { std::cout << "Success" << std::endl; });
fq.execute();
}
3 Answers 3
This code is missing some definitions on which it depends:
#include <cstddef>
#include <utility>
#include <vector>
using std::size_t;
We have
void add(Func&& func)
FuncWrapper<Func>{std::move(func)};
This looks wrong to me - forwarding references normally pair with std::forward()
- not std::move()
, which pairs with value objects.
In any case, the code seems unwieldy and unnecessary. You say you're trying to "reduce the number of allocations", but it's not clear that allocation is a bottleneck for the program. The code we have here seems disproportionate for an unproven requirement - otherwise known as premature optimisation.
It would be simpler use std::function
for type erasure, giving much more maintainable code:
#include <functional>
#include <utility>
#include <vector>
class FuncQueue
{
std::vector<std::function<void(void)>> queue = {};
public:
template<typename... Args>
void emplace_back(Args&&... args)
{
queue.emplace_back(std::forward<Args>(args)...);
}
// N.B. no locking, so don't call from different threads
void execute()
{
for (auto const& func: queue) {
if (func) {
func();
}
}
queue.clear();
}
};
To be honest, I wouldn't even create a class for this; we just need a simple function; a type alias makes it somewhat easier, too:
#include <functional>
#include <vector>
using func_queue = std::vector<std::function<void(void)>>;
void execute(func_queue& queue)
{
for (auto const& func: queue) {
if (func) {
func();
}
}
queue.clear();
}
Then there's only a slight modification to how we use it:
#include <iostream>
int main()
{
func_queue fq;
fq.reserve(4);
fq.emplace_back([]{ std::cout << "3\n"; });
fq.emplace_back([]{ std::cout << "2\n"; });
fq.emplace_back([]{ std::cout << "1\n"; });
fq.emplace_back([]{ std::cout << "Success\n"; });
execute(fq);
}
Start simple
You are trying to do too much at once. Start with creating a function queue from building blocks from the standard library:
class FunctionQueue {
std::queue<std::function<void()>> queue;
public:
void add(std::function<void()> function) {
queue.push(function);
}
void execute() {
while (!queue.empty()) {
queue.front()();
queue.pop();
}
}
};
This is already something that works. It needs some work to avoid unnecessary copies, but see Toby's answer for that. The only advantage of your code is that it allows storing functions of varying size more compactly in a single vector of bytes. However, do you need this though? I see some alternatives:
- Just use
std::function<>
, which might do heap allocations sometimes. Then again, yourstd::vector<std::byte>
also does multiple heap allocations, but just less of them. - Create an alternative to
std::function<>
that has a larger size and thus reduces the amount of heap allocations, at the cost of more memory usage for "small" functions. It keeps the queue itself simple though. - Create an alternative to
std::function<>
that uses a custom allocator, which can be used to allocate from a vector instead of from heap memory.
Be careful when manually constructing objects
You do placement-new
for the function objects, and you destroy them in execute()
. But what if execute()
is never called? Make sure you clean up properly in all cases. You could add a member function named clear()
that does this, and then call it from execute()
and from the destructor of FuncQueue
.
Your add
function is completely broken for two reasons:
data_.resize(data_.size() + sizeof(FuncWrapper<Func>));
This may causedata_
to allocate new storage and move the objects it contains into the new storage, but the the objects it contains are juststd::byte
s. The actualFuncWrapper
(and thus theFunc
objects they contain) will never have their copy or move constructors called. You need to manually move all of your objects ifdata_
would allocate new storage.Func&&
is a forwarding reference, not an rvalue reference. That means thatFunc
may be a reference type ifadd
is passed an lvalue. In that caseFuncWrapper<Func>
will only hold a reference to the object referenced byfunc
. That object could easily go out of scope beforeexecute
is called.
The following version resolves those issues by
- If
data_
would need to allocate new storage, it creates a new, larger vector and manually moves the existing objects into it - Applying
std::decay
toFunc
and using the resulting decayed type instead of usingFunc
directly
class FuncWrapperInterface
{
public:
// ...
virtual void move_to(std::byte* new_location) = 0;
};
template<typename Func>
class FuncWrapper : public FuncWrapperInterface
{
public:
// ...
void move_to(std::byte* new_location) override
{
new (new_location) FuncWrapper(std::move(*this));
}
// ...
};
class FuncQueue
{
public:
template<typename Func>
void add(Func&& func)
{
using FuncType = std::decay_t<Func>;
auto start{data_.size()};
if (start + sizeof(FuncWrapper<FuncType>) > data_.capacity()) {
std::vector<std::byte> new_data;
new_data.reserve(start * 2);
for (std::size_t i = 0; i < start;) {
auto f = std::launder(reinterpret_cast<FuncWrapperInterface*>(data_.data() + i));
new_data.insert(new_data.begin() + i, f->size(), std::byte{0});
f->move_to(new_data.data() + i);
i += f->size();
f->~FuncWrapperInterface();
}
data_ = std::move(new_data);
}
data_.insert(data_.begin() + start, sizeof(FuncWrapper<FuncType>), std::byte{0});
new (data_.data() + start) FuncWrapper<FuncType>{std::forward<Func>(func)};
}
// ...
};
-
\$\begingroup\$ The placement
new
in OP's code will cause theFuncWrapper
object to be move-constructed inside the vector of bytes. \$\endgroup\$G. Sliepen– G. Sliepen2024年09月15日 13:22:03 +00:00Commented Sep 15, 2024 at 13:22 -
1\$\begingroup\$ For the new one being added, yes -- but not so for the existing ones already in the vector. Without guarantees that the
FuncWrapper
type is blittable (safe to memcpy without calling the move constructor) -- a guarantee that is hard to make for a template class -- this may be unsafe. Consider the case where the type contains pointers to its own members, for example. This may even be occurring behind your back in a vtable or typeinfo block. \$\endgroup\$Miral– Miral2024年09月16日 01:15:28 +00:00Commented Sep 16, 2024 at 1:15 -
\$\begingroup\$ @G.Sliepen Check my first example. It not only shows the pointer-into-self issue, but it also segfaults (I believe due to the way short string optimization is implemented). \$\endgroup\$Miles Budnek– Miles Budnek2024年09月16日 05:29:41 +00:00Commented Sep 16, 2024 at 5:29
void Command(int arg) { data.write(eCommandId); data.write(arg); }
Executor:void Exec() { Command c = readCommand(); switch(c)...
\$\endgroup\$