Changeset View
Standalone View
source/simulation2/system/ComponentDataHolder.h
- This file was added.
/* Copyright (C) 2022 Wildfire Games. | |||||||||||||
* 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 <vector> | |||||||||||||
Stan: std::byte. | |||||||||||||
// this is a pool allocator for a type with constant size | |||||||||||||
class CComponentDataHolder | |||||||||||||
{ | |||||||||||||
Done Inline ActionsDoes 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? | |||||||||||||
Done Inline ActionsYes 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 Mercury: Yes if we want to reuse this code for other types of objects. This is a type of allocator… | |||||||||||||
Done Inline ActionsMaybe. 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… | |||||||||||||
Done Inline ActionsI have no opinion about the name. Mercury: I have no opinion about the name. | |||||||||||||
Not Done Inline ActionsSeconded 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… | |||||||||||||
Done Inline ActionsWould we be templating IComponent* or the actual component type? Mercury: Would we be templating IComponent* or the actual component type? | |||||||||||||
private: | |||||||||||||
Done Inline Actions
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… | |||||||||||||
Done Inline ActionsComment changed. Not sure where that ENSURE would go, probably not needed. Mercury: Comment changed. Not sure where that ENSURE would go, probably not needed. | |||||||||||||
Not Done Inline ActionsDon't need this if you have doxygen I suppose. Stan: Don't need this if you have doxygen I suppose. | |||||||||||||
void* m_Start; | |||||||||||||
void* m_Back; | |||||||||||||
void* m_End; | |||||||||||||
size_t m_Offset; | |||||||||||||
size_t m_PoolSize; | |||||||||||||
std::vector<void*> m_OpenSlots; | |||||||||||||
Done Inline Actionsinclude<vector> That's a lot of void* can't we have more specific types? Stan: include<vector>
That's a lot of void* can't we have more specific types?
Do we need smart ptrs? | |||||||||||||
Done Inline Actionsinclude <vector>: void*:
smart pointer: Mercury: include <vector>:
will fix
void*:
At one point I had them as unsigned char pointers. I… | |||||||||||||
Done Inline ActionsDisregard 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… | |||||||||||||
Done Inline Actionsstd::byte* would propably be more apropriate. phosit: std::byte* would propably be more apropriate. | |||||||||||||
public: | |||||||||||||
Done Inline ActionsDoes it make sense to store end _and_ size? phosit: Does it make sense to store end _and_ size? | |||||||||||||
Done Inline ActionsEnd is stored to avoid recalculation on every allocation. Mercury: End is stored to avoid recalculation on every allocation. | |||||||||||||
Not Done Inline Actionscould they be packed in to a PS::span phosit: could they be packed in to a `PS::span` | |||||||||||||
Done Inline ActionsSo 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… | |||||||||||||
CComponentDataHolder(size_t size, size_t align, uint32_t capacity = 10000); | |||||||||||||
~CComponentDataHolder(); | |||||||||||||
void ResetState(); | |||||||||||||
void* Allocate(); | |||||||||||||
void Deallocate(IComponent* ptr); | |||||||||||||
Done Inline ActionsDo 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… | |||||||||||||
Done Inline Actions10,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… | |||||||||||||
Done Inline ActionsDoes capacity represent initial capacity? Or maximum capacity? jprahman: Does capacity represent initial capacity? Or maximum capacity? | |||||||||||||
Done Inline ActionsInitial, there is no max. Mercury: Initial, there is no max. | |||||||||||||
void* Start(); | |||||||||||||
size_t Offset(); | |||||||||||||
void* Back(); | |||||||||||||
}; | |||||||||||||
Not Done Inline ActionsWhy not use std::vector<const std::unique_ptr<std::byte[]>>? phosit: Why not use std::vector<const std::unique_ptr<std::byte[]>>? | |||||||||||||
Not Done Inline Actions
Mercury: 1. I feel the life cycle of the allocation is deterministic enough that it isn't needed.
2. I… | |||||||||||||
Not Done Inline Actionswith 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… | |||||||||||||
struct SComponentDataGenerator | |||||||||||||
{ | |||||||||||||
private: | |||||||||||||
void* m_Ptr; | |||||||||||||
CComponentDataHolder* m_Data; | |||||||||||||
public: | |||||||||||||
SComponentDataGenerator(CComponentDataHolder* data); | |||||||||||||
IComponent* Next(); | |||||||||||||
void Reset(); | |||||||||||||
}; | |||||||||||||
Done Inline ActionsTechnically 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… | |||||||||||||
Done Inline ActionsComponentDataHolders 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. | |||||||||||||
#endif // INCLUDED_COMPONENTDATAHOLDER |
std::byte.