Differential D4718 Diff 20746 source/simulation2/system/ComponentDataHolder.h

Changeset View

Standalone View

View Options

source/simulation2/system/ComponentDataHolder.h

This file was added.

* This file is part of 0 A.D.

* 0 A.D. is free software: you can redistribute it and/or modify

* it under the terms of the GNU General Public License as published by

* the Free Software Foundation, either version 2 of the License, or

* (at your option) any later version.

* 0 A.D. is distributed in the hope that it will be useful,

* but WITHOUT ANY WARRANTY; without even the implied warranty of

* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

* GNU General Public License for more details.

* You should have received a copy of the GNU General Public License

* along with 0 A.D. If not, see <http://www.gnu.org/licenses/>.

#ifndef INCLUDED_COMPONENTDATAHOLDER

#define INCLUDED_COMPONENTDATAHOLDER

#include "simulation2/system/IComponent.h"

#include <cstddef>

StanUnsubmitted

Done

#include "simulation2/system/IComponent.h"

+ #include <cstddef>

#include <vector>

// this is a pool allocator for a type with constant size

std::byte.

Stan: std::byte.

#include <vector>

// this is a pool allocator for a type with constant size

StanUnsubmitted

Done

Does it make sense to make this a template and use T = IComponent?

Stan: Does it make sense to make this a template and use T = IComponent?

MercuryAuthorUnsubmitted

Done

Yes if we want to reuse this code for other types of objects. This is a type of allocator called a pool allocator.

The pool allocator defined in source/lib/allocators/pool.cpp handles objects of
arbitrary size where as this one requires that all objects in a single allocator object are the same size ( or at least not larger then a given size ). So maybe that allocator could be replaced with this one but it wouldn't be a drop in replacement. I don't know how hard it would be as I haven't read the relevant code yet. Seems to me like any attempt to do this should be a separate patch.

Mercury: Yes if we want to reuse this code for other types of objects. This is a type of allocator…

StanUnsubmitted

Done

Maybe. Might be worth a ticket if this one gets committed :)

Maybe this one should also have pool in the name.

Stan: Maybe. Might be worth a ticket if this one gets committed :) Maybe this one should also have…

MercuryAuthorUnsubmitted

Done

I have no opinion about the name.

Mercury: I have no opinion about the name.

jprahmanUnsubmitted

Not Done

Seconded on templating on the underlying type, can probably reduce/eliminate some of the casting by updating the type of the underlying member types

jprahman: Seconded on templating on the underlying type, can probably reduce/eliminate some of the…

MercuryAuthorUnsubmitted

Done

Would we be templating IComponent* or the actual component type?

Mercury: Would we be templating IComponent* or the actual component type?

class CComponentDataHolder final

StanUnsubmitted

Done

#include <vector>

- // this is a pool allocator for a type with constant size

+ /**

+ * This is a pool allocator for a type with constant size.

+ */

class CComponentDataHolder final

Could use doxygen so it shows up on https://docs.wildfiregames.com/

Technically should be this, but we don't use that anywhere.

/**
 * @class CComponentDataHolder
 * @brief This is a pool allocator for a type with constant size.
 */

Should we have an ENSURE/static_assert for the constant size requirement?

Stan: Could use doxygen so it shows up on https://docs.wildfiregames.com/ Technically should be this…

MercuryAuthorUnsubmitted

Done

Comment changed. Not sure where that ENSURE would go, probably not needed.

Mercury: Comment changed. Not sure where that ENSURE would go, probably not needed.

StanUnsubmitted

Not Done

Don't need this if you have doxygen I suppose.

Stan: Don't need this if you have doxygen I suppose.

{

private:

std::byte* m_Back;

std::byte* m_End;

size_t m_SlotSize;

size_t m_PoolSize;

StanUnsubmitted

Done

include<vector>

That's a lot of void* can't we have more specific types?
Do we need smart ptrs?

Stan: include<vector> That's a lot of void* can't we have more specific types? Do we need smart ptrs?

MercuryAuthorUnsubmitted

Done

include <vector>:
will fix

void*:
At one point I had them as unsigned char pointers. I switched to void because it means

We can use static_cast instead of reinterpret_cast, which may help performance a bit.
These variables are storing raw memory addresses with no type so I think void pointer is a good description of what they are rather then a hack.

smart pointer:
The pool memory is released when the ComponentManager::ResetState method runs, so leaks seem unlikely to me.

Mercury: include <vector>: will fix void*: At one point I had them as unsigned char pointers. I…

MercuryAuthorUnsubmitted

Done

Disregard that last part, sorry. I meant to say the pools are released when the component manager destructor is called but then realized I hadn't done that lol. Fixed now.

Mercury: Disregard that last part, sorry. I meant to say the pools are released when the component…

phositUnsubmitted

Done

std::byte* would propably be more apropriate.

phosit: std::byte* would propably be more apropriate.

std::vector<std::byte*> m_OpenSlots;

phositUnsubmitted

Done

Does it make sense to store end _and_ size?

phosit: Does it make sense to store end _and_ size?

MercuryAuthorUnsubmitted

Done

End is stored to avoid recalculation on every allocation.

Mercury: End is stored to avoid recalculation on every allocation.

phositUnsubmitted

Not Done

could they be packed in to a PS::span

phosit: could they be packed in to a `PS::span`

MercuryAuthorUnsubmitted

Done

So storing a span with the beginning set to the pool pointer and the end set to end? Yes but why?

Mercury: So storing a span with the beginning set to the pool pointer and the end set to end? Yes but…

std::vector<std::byte*> m_Pools;

void MakePool();

CComponentDataHolder(const CComponentDataHolder&) = delete;

CComponentDataHolder & operator=(const CComponentDataHolder&) = delete;

public:

jprahmanUnsubmitted

Done

Do we have any sense on what practical size is best as a default capacity? 10000 feels like a magic number. Also, if this were templated on the underlying type, then the size and align parameters would be redundant because the size + alignment requirement could be inferred from the template type parameter

jprahman: Do we have any sense on what practical size is best as a default capacity? 10000 feels like a…

MercuryAuthorUnsubmitted

Done

10,000 is enough to not have to allocate a second pool for any component on 4v4 300 max pop mainland large. Very Large and Giant require more space for pathing nodes. It could be reduced but it's not that much RAM anyway.

100 Component Types * 64 bytes average size per component (estimated, probably way to high) * 10,000 = 64MB

Mercury: 10,000 is enough to not have to allocate a second pool for any component on 4v4 300 max pop…

jprahmanUnsubmitted

Done

Does capacity represent initial capacity? Or maximum capacity?

jprahman: Does capacity represent initial capacity? Or maximum capacity?

MercuryAuthorUnsubmitted

Done

Initial, there is no max.

Mercury: Initial, there is no max.

CComponentDataHolder(size_t size, size_t align, uint32_t capacity = 10000);

~CComponentDataHolder();

void ResetState();

std::byte* Allocate();

void Deallocate(IComponent* ptr);

phositUnsubmitted

Not Done

Why not use std::vector<const std::unique_ptr<std::byte[]>>?

phosit: Why not use std::vector<const std::unique_ptr<std::byte[]>>?

MercuryAuthorUnsubmitted

Not Done

I feel the life cycle of the allocation is deterministic enough that it isn't needed.

I don't know how. Tried to figure it out for a bit. Maybe will try some more.

Mercury: 1. I feel the life cycle of the allocation is deterministic enough that it isn't needed. 2. I…

phositUnsubmitted

Not Done

with unique_ptr the destructor could be default. If you want to stay with a user defined destructor it should be noexcept.

speaking of special member function the copy/move constructor/assignment should be deleted. m_End and m_Back would point in the old object.

phosit: with unique_ptr the destructor could be default. If you want to stay with a user defined…

std::vector<std::byte*> Pools() const;

size_t SlotSize() const;

size_t PoolSize() const;

std::byte* Back() const;

};

struct SComponentDataGenerator final

{

private:

std::byte* m_Ptr;

jprahmanUnsubmitted

Done

Technically there is a risk the SComponentDataGenerator could outlive the CComponentDataHolder this pointer refers to. Worth a comment that SComponentDataGenerator is really meant to be short-lived for iteration and not to hang onto it. Traditionally I'd use a shared/weak_ptr<> here, but that's a perf hit due to the std::atomic ref count on the shared_ptr<> which I really don't like.

jprahman: Technically there is a risk the `SComponentDataGenerator` could outlive the…

MercuryAuthorUnsubmitted

Done

ComponentDataHolders have the same life time as ComponentManager and thus the game it's self. If one got destructed mid-game we would have many unrecoverable problems.

Mercury: ComponentDataHolders have the same life time as ComponentManager and thus the game it's self.

const CComponentDataHolder* m_Data;

size_t m_PoolIndex;

std::byte* m_Back;

size_t m_LastIndex;

void SetPoolIndex(size_t index);

public:

explicit SComponentDataGenerator(const CComponentDataHolder* data);

IComponent* Next();

void Reset();

};

#endif // INCLUDED_COMPONENTDATAHOLDER