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

Call for comments: make EnTT allocator-aware #22

Closed
skypjack opened this issue Nov 21, 2017 · 21 comments
Closed

Call for comments: make EnTT allocator-aware #22

skypjack opened this issue Nov 21, 2017 · 21 comments
Assignees
Labels
discussion it sounds interesting, let's discuss it enhancement accepted requests, sooner or later I'll do it

Comments

@skypjack
Copy link
Owner

skypjack commented Nov 21, 2017

I was thinking about making EnTT allocator-aware. It's not the easiest task ever, but it could be an interesting feature.

To do that, the first thing that comes to my mind is to add an extra template parameter to the registry that is propagated down to the internal data structures if required. Moreover, I would add a reserve function to the registry to pre-allocate enough space for components and entities.
This way, users that don't want to deal with memory management can freely ignore the fact that EnTT is allocator-aware and let it use an std::allocator as it already does.

The other solution around is to define an interface for allocators that offers a few virtual member functions, then pass an opaque instance to the constructor of the registry.
I don't like that much this approach, for it looks to me much more invasive than the other one.

If anyone out there is listening, every comment is welcome as usual.

@lessness
Copy link

First consideration: EnTT is focused on performances
Second consideration: at the same time, EnTT has a clear API.

Introducing an interface with virtual methods sounds a bit against point one (opinion based), so I would like the first approach proposed (introduce a new template parameter).
But allocators are nasty beasts (opinion based), one may want to partially specialize allocations, for example (opinion based). My proposal is: introduce a new template parameter struct that let the user specify different allocators for different types; those types without a specified allocator will use the std::allocator. The registry will be injected with such a structure, defaulted to an empty one. This solution is a bit against point 2 above.
Another solution (opinion based): instead of "polluting" the registry with the allocator, introduce a new object to be set up with the struct above on library start-up, if needed. Why? Because it is not a registry responsibility to set an allocator (opinion based).

@tazio
Copy link

tazio commented Nov 21, 2017

As a side point: std::allocator's interface was not made to encapsulate allocation algorithms, but to hide near/far pointers that existed in 1993. So if you want an allocator, you might consider an entirely different interface as well.

@skypjack
Copy link
Owner Author

@lessness
The registry uses internally a bunch of sparse sets and allocations of storage for components boil down to them. That's why I think making the registry allocator-aware is the right choice. Otherwise we need a full redesign of EnTT. Do you see any other viable solution?

@tazio
Well, that's what C++ offers nowadays. One could also consider pmr:: namespace and polymorphic allocators. However I'm not that sure it's a good choice, for it introduces virtual calls all around the codebase by design. Have you any other suggestion regarding an entirely different interface? I can't see exactly what you have in mind.

@lessness
Copy link

On second thought, not injecting the registry with the allocator struct info is possible only if you introduce a global class/object, and if the registry uses it internally to retrieve the allocators. This does not require a full redesign, but involves a lot of rewriting; I don't know whether this is an appropriate solution.

@skypjack
Copy link
Owner Author

@lessness
Got it. Being EnTT a header-only library, I don't like that much the idea of pushing around global things the users are not fully aware of. However this is a viable solution, so let's put it aside and see if that's the preferred one.

@lessness
Copy link

That given, I would go for injecting directly the registry.
I agree (opinion based), global objects do not smell so tasty at this point.

@skypjack skypjack added discussion it sounds interesting, let's discuss it enhancement accepted requests, sooner or later I'll do it labels Nov 26, 2017
@dbacchet
Copy link

hi guys, sorry for the late reply.
Personal opinion here: allocators highly depend on the way you want to consume components from the systems, so I feel that should be possible to specify them at the component level (for example transforms could use an aligned chunk of memory on the stack, while mesh data a dynamic heap alloc).
Assuming that creation/destruction is much less frequent than accessing the component, I feel like the flexibility of having a basic allocator interface like the following is well worth the cost of an additional memory lookup for the virtual functions:

class Allocator {
public:
    virtual void *allocate(size_t size, size_t align) = 0;
    virtual void deallocate(void *p) = 0;
}

Also, we should keep in mind that most of the cost of allocating memory is in the OS side, for synchronization etc, so the table lookup will not add much to that.

Keep up with the great job, this library is getting better every day!

@skypjack
Copy link
Owner Author

@dbacchet

As far as I can see, what you suggest is a breaking change.
The Registry has to accept a reference to an allocator and thus already existent code won't work anymore. Something like this:

Registry(Alloc &alloc): allocator{alloc} {}

Am I wrong?


Furthermore, a journey along this path suggests that EnTT should become a real library.
I know that a header-only structure is desirable. However, we are adding too many things to support this thesis.

Thoughts?

@DJuego
Copy link

DJuego commented Dec 12, 2017

I am not an advanced c++ developer. And i (still) have not a real experience with EnTT. So In this moment I would not be affected by breaking changes.

I must admit i like very, very, very much the header-only feature. Simplify things a lot and it makes portabily easier. I prefer small and perfect gems that humongous and super-general beasts (I'm looking at you, Boost). In fact I collect those gems. And that's how I discovered, EnTT!! (the header-only feature). :-D

In my humble opinión the only valid reason to give up that feature (header-only) is to improve performance significantly.

On the other hand It seems dangerous to implement new-and-very-shiny-and-cool features before they are requested by genuine use cases (similar to premature optimization). However perhaps i miss the global overview...

DJuego

P.S: I like the EnTT allocator-aware feature. 👍 It is interesting.

@dbacchet
Copy link

dbacchet commented Dec 14, 2017

@skypjack I was thinking about having the possibility to specify a different allocator for each component type; something on the line of an internal map type-allocator.

The API could something similar to:

registry.set_allocator<Transform>(my_custom_allocator);

defaulting to a default allocator in case the user did not specify one for the given type.

I don't consider this very high priority though; EnTT is quite fast already :)

@skypjack
Copy link
Owner Author

skypjack commented Dec 15, 2017

@dbacchet @DJuego

It looks like C++17 and thus the standard template library offers exactly what we are discussing. It's in the namespace pmr.
I plan to have a go with it. Any thought?

@skypjack skypjack self-assigned this Dec 24, 2017
@dbacchet
Copy link

dbacchet commented Jan 6, 2018

C++17 will not work for me (using C++14 is already pushing the boundaries of the compilers I have to support, unfortunately...) for a while.

@skypjack
Copy link
Owner Author

I added the request to the TODO file. I'm closing the issue because I've all the information I need to start working on it sooner or later.
Making EnTT allocator-aware is a long term task at the moment. I don't really need it actually for my projects, but it's something I'd like to do for the future.
So, stay tuned, a day in the future I'll work on it for sure!! :-)

@rileylev
Copy link

rileylev commented Aug 7, 2019

I'm taking a shot at adding allocator awareness to some of the files in the entity folder. So far all the tests still pass!

@skypjack
Copy link
Owner Author

skypjack commented Aug 7, 2019

@rilerez is your work publicly available? I've a pretty clear idea of what I'd like to do in this sense, even though I haven't done it yet. We can discuss your changes to see if they match eventually.

@rileylev
Copy link

rileylev commented Aug 7, 2019

I haven't pushed anything yet but I will push it to the experimental branch of my enTT fork. So far I have just been adding an optional pmr::memory_resource argument to your classes that allocate. These get passed to the pmr::vectors I used in place of std::vector.

@philippecp
Copy link

Probably coming in a bit late here, but a nice scenario to support would be arena allocators that don't return memory incrementally but rather in one big reset thus avoid the expense of managing memory (and get better locality too since memory allocations won't be fragmented!).

@skypjack
Copy link
Owner Author

@philippecp A custom allocator you can set on a per-type basis will allow this. What's wrong with it?

@stefan-pdx
Copy link

stefan-pdx commented Mar 26, 2021

Hello!

I have a use-case where I'd like to have EnTT allocate objects within a region of shared memory that is accessible via other processes. T* cannot be used because each process contain its own view of shared memory via a virtual address space, so a pointer written to shared memory in one process wouldn't be valid in another. This was the motivation for Boost's offset_ptr<T>, an address independent pointer that works well for representing pointers in a single contiguous address range in shared memory.

However, since I'm working with multiple blocks of shared memory, I've had to resort to a custom "fancy pointer" that encodes a block pointer (block_id, offset) (similar to what ZFS does for its block pointers). PMR doesn't help because its allocate() method has to return a T*.

C++17 seemed to formalize Allocator as a named requirement. This allows the allocator to provide the pointer type that a container implementation would use. For example:

template <typename T>
struct my_allocator {
   using pointer = block_pointer<T>;
   // ...
}

template <typename T, typename Allocator = std::allocator<T>>
struct my_container {
   using pointer = typename Allocator::pointer;

   // Use `pointer` instead of `T*`
}

my_allocator allocator;

my_container<int, std::allocator_traits<my_allocator>> container(allocator);

(Side note: you can also implement fancy pointers through std::pointer_traits, but for my use-case, it makes sense to couple the fancy pointer with the allocator implementation.)

So, from my perspective, the first step in making EnTT allocator-aware is to move away from using raw pointers and using the pointer_type provided by a custom allocator. A good starting point would be basic_storage::storage_iterator::pointer:

template<typename Entity, typename Type, typename Allocator = std::allocator<Type>, typename = void>
class basic_storage: public basic_sparse_set<Entity> {
    template<typename Value>
    class storage_iterator final {
        ...

    public:
        using value_type = Value;
        using pointer = Allocator::pointer; // <-- Change from `value_type*` to `Allocator::pointer`
        ...
    }
}

I believe this approach would be compatible with std::allocator, PMR, and allocators that leverage fancy pointers.

Please let me know if there are any details you'd like for me to elaborate on.

Thanks!

@skypjack
Copy link
Owner Author

@stefan-pdx maybe it's worth it to create a discussion for this.
At a first glance, what I don't get from what you suggest is how you would pass the allocator to your pool. It fixes the pointer type but still, you don't have a way to initialize the pool with the right params (yet).
Am I missing something?

@stefan-pdx
Copy link

Thanks @skypjack! I'll follow up in a separate discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion it sounds interesting, let's discuss it enhancement accepted requests, sooner or later I'll do it
Projects
None yet
Development

No branches or pull requests

8 participants