Page MenuHomeWildfire Games
Differential D4718 Diff 20568 source/simulation2/system/ComponentDataHolder.h

Changeset View

Standalone View

View Options

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>
StanUnsubmitted
Done
Inline Actions
#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.
// this is a pool allocator for a type with constant size
class CComponentDataHolder final
{
StanUnsubmitted
Done
Inline Actions

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
Inline Actions

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
Inline Actions

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
Inline Actions

I have no opinion about the name.

Mercury: I have no opinion about the name.
jprahmanUnsubmitted
Not Done
Inline Actions

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
Inline Actions

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

Mercury: Would we be templating IComponent* or the actual component type?
private:
StanUnsubmitted
Done
Inline Actions
#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
Inline Actions

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
Inline Actions

Don'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;
StanUnsubmitted
Done
Inline Actions

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
Inline Actions

include <vector>:
will fix

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

  1. We can use static_cast instead of reinterpret_cast, which may help performance a bit.
  2. 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
Inline Actions

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
Inline Actions

std::byte* would propably be more apropriate.

phosit: std::byte* would propably be more apropriate.
public:
phositUnsubmitted
Done
Inline Actions

Does it make sense to store end _and_ size?

phosit: Does it make sense to store end _and_ size?
MercuryAuthorUnsubmitted
Done
Inline Actions

End is stored to avoid recalculation on every allocation.

Mercury: End is stored to avoid recalculation on every allocation.
phositUnsubmitted
Not Done
Inline Actions

could they be packed in to a PS::span

phosit: could they be packed in to a `PS::span`
MercuryAuthorUnsubmitted
Done
Inline Actions

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…
CComponentDataHolder(size_t size, size_t align, uint32_t capacity = 10000);
~CComponentDataHolder();
void ResetState();
void* Allocate();
void Deallocate(IComponent* ptr);
jprahmanUnsubmitted
Done
Inline Actions

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
Inline Actions

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
Inline Actions

Does capacity represent initial capacity? Or maximum capacity?

jprahman: Does capacity represent initial capacity? Or maximum capacity?
MercuryAuthorUnsubmitted
Done
Inline Actions

Initial, there is no max.

Mercury: Initial, there is no max.
void* Start() const;
size_t Offset() const;
void* Back() const;
};
phositUnsubmitted
Not Done
Inline Actions

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
Inline Actions
  1. I feel the life cycle of the allocation is deterministic enough that it isn't needed.
  1. 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
Inline Actions

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…
struct SComponentDataGenerator final
{
private:
void* m_Ptr;
CComponentDataHolder* m_Data;
public:
SComponentDataGenerator(CComponentDataHolder* data);
IComponent* Next();
void Reset();
};
jprahmanUnsubmitted
Done
Inline Actions

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
Inline Actions

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.
#endif // INCLUDED_COMPONENTDATAHOLDER
Wildfire Games · Phabricator