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

Make single link node pool unlimited #3986

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

TheNormalnij
Copy link
Member

Fixes crash with offset 0x00133606.
The crash related to the high density of buildings.

@TheNormalnij TheNormalnij added enhancement New feature or request bugfix Solution to a bug of any kind and removed enhancement New feature or request labels Jan 28, 2025
@TheNormalnij TheNormalnij force-pushed the TheNormalnij/single_node_pool_resize branch from 23eaf01 to 31c5d72 Compare January 28, 2025 21:21
@tederis
Copy link
Member

tederis commented Jan 30, 2025

You did a lot of work and it's great. I just want to understand why did you choose creating CDynamicPool instead of just using std::vector? In the context of this PR std::vector allows to find elements with O(1) complexity(by comparing data pointers), it also allows you to remove elements with O(1) complexity(swap and pop ideom). std::vector has to be a reasonable choice here.

Okay, I see that memory pieces have to be persistent and we cannot use swap and pop ideom here.

@Dutchman101
Copy link
Member

Fixes crash with offset 0x00133606. The crash related to the high density of buildings.

This crash is exceedingly rare on MTA (measured by the latest build with a high spread; crash stats) so to judge the PR's feasability it would be good to know if it's worth it, as in: are there no negative/side effects at play, like a global increase of RAM usage as the pool allocates more? Is it dynamically expanding and going back down depending on a servers' usage of map and buildings, so that as long the usage isn't huge, there are no negative effects?

In the latter case it wouldn't be too bad to have it in. If it has no negative effects, proportionality to what it aims to fix isn't a thing.

@TheNormalnij If you could please make a list of any effects you expect from this change?

@TheNormalnij
Copy link
Member Author

Fixes crash with offset 0x00133606. The crash related to the high density of buildings.

This crash is exceedingly rare on MTA (measured by the latest build with a high spread; crash stats) so to judge the PR's feasability it would be good to know if it's worth it, as in: are there no negative/side effects at play, like a global increase of RAM usage as the pool allocates more? Is it dynamically expanding and going back down depending on a servers' usage of map and buildings, so that as long the usage isn't huge, there are no negative effects?

In the latter case it wouldn't be too bad to have it in. If it has no negative effects, proportionality to what it aims to fix isn't a thing.

@TheNormalnij If you could please make a list of any effects you expect from this change?

This crash is rare because developers use createObjects instead of createBuilding when their map crashes.
I'm trying to increase the building limit because buildings fix fps issues on big maps. This allows more creative maps with better lighting.
This PR doesn't cause significant memory usage. I'll implement TEDERIS's suggestion, which requires an additional ~200 KB on game startup. createBuilding is still more memory efficient. 10k buildings use 1.5 MB less than 10k objects. The pool resets capacity on reconnect and can be resized manually via engineSetPoolCapacity.

The pool implementation is very universal. I plan to use it in future fixes too.

Copy link
Contributor

@Pirulax Pirulax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like that a traditional linked list of these "part"s would be better than having a vector of them, especially when allocating a free-list is much better in terms of performance than a basically O(N) lookup.

Client/game_sa/CDynamicPool.h Outdated Show resolved Hide resolved
Client/game_sa/CDynamicPool.h Outdated Show resolved Hide resolved
Client/game_sa/CDynamicPool.h Outdated Show resolved Hide resolved
@TheNormalnij
Copy link
Member Author

I feel like that a traditional linked list of these "part"s would be better than having a vector of them, especially when allocating a free-list is much better in terms of performance than a basically O(N) lookup.

The performance cost for allocating m_poolParts isn't really important here. But I agree std::vector<std::unique_ptr<CDynamicPoolPart<PoolObjT>>> is weird. It should be std::list<CDynamicPoolPart<PoolObjT>> or std::vector<DynamicPoolPart<PoolObjT>>. The second variant causes less memory fragmentation.

@tederis
Copy link
Member

tederis commented Feb 2, 2025

The std::vector was fine because an array of CDynamicPoolPart ptrs isn't something frequently changing. Technically std::list is also okay, but for small types like uint or std::unique_ptr it performs worse because of cache misses and memory fragmentation. In most cases std::vector is the best choice when you need to iterater over an array of small fixed(non-volatile) objects. But nvm, I just wanted to note that your previous solution was also fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Solution to a bug of any kind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants