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

Switch droids to use PagedEntityContainer<DROID> as backing storage #3689

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

ManManson
Copy link
Member

Aside from changing droids to be allocated via a seprated PagedEntityContainer<DROID>, this changeset also implements a couple of other important things:

  • The number of elements per page in PagedEntityContainer can now be customized
    via MaxElementsPerPage template argument. The default value is 1024 elements.
    NOTE: The storage for droids uses 256 elements per page.
  • PagedEntityContainer can now be configured not to reuse slots upon element erasure. In such case, the slot becomes invalid right after first use. The behavior is controlled by ReuseSlots template argument. The default value is true.
    NOTE: The storage for droids disable slot reuse feature, at least until we are sure that other memory-related and dependency-tracking issues between DROID instances are eliminated throughout the WZ code base.

This PR should not introduce any notable performance difference. Although, memory utilization patterns should become slightly better (e.g. there should be less memory fragmentation overall).

Signed-off-by: Pavel Solodovnikov [email protected]

The default value is 1024 elements per page.

Signed-off-by: Pavel Solodovnikov <[email protected]>
@ManManson ManManson requested review from past-due and KJeff01 March 16, 2024 18:31
@ManManson ManManson added this to the 4.5.0-beta1 milestone Mar 16, 2024
@ManManson ManManson force-pushed the droids_optimized_storage_alt branch from 7488005 to 786cd32 Compare March 16, 2024 19:45
This allows to disable slot reuse in `PagedEntityContainer`.

The feature is temporary and will be used only until
we are confident that we don't have any issues in the
WZ code base regarding object pointers not updated
properly, e.g. when transitioning between the base
and offworld missions.

So, it's to prevent cases where one object would be
overwritten by another and the game code haven't noticed
that. It's better to crash/assert rather than silently
use one object instead of another, which could lead to
unobvious and weird bugs.

Signed-off-by: Pavel Solodovnikov <[email protected]>
1. Split the droid storage into pages containing 256 droids
2. Disable slot reuse to guard against memory-related issues
   when some object pointers won't get updated properly,
   e.g. when transitioning between the base and offworld missions.

Signed-off-by: Pavel Solodovnikov <[email protected]>
@ManManson ManManson force-pushed the droids_optimized_storage_alt branch from 786cd32 to de6a149 Compare March 16, 2024 19:46
@past-due
Copy link
Member

past-due commented Mar 19, 2024

I've pushed a commit to Clear the PagedEntityContainer<DROID> in objmemShutdown() (so memory usage doesn't just continue to grow even after shutting down one match and starting another).

However I still have a concern that, in a long-duration campaign play session for example (progressing through many levels of the campaign), we'll just keep allocating blocks and never freeing them.

I think it might be helpful for PagedEntityContainer to free a block of memory once all of its elements are invalidated, and allocate a new one (when ReuseSlots == false, of course). Maybe by calling allocate_storage(); reset_metadata(); on the page that's fully invalidated?
(Perhaps we can make that another template parameter, if needed, and enable it by default.)

(Only in debug development situations should we just accumulate memory and never free it, IMHO.)

@ManManson
Copy link
Member Author

ManManson commented Mar 20, 2024

I think it might be helpful for PagedEntityContainer to free a block of memory once all of its elements are invalidated, and allocate a new one (when ReuseSlots == false, of course). Maybe by calling allocate_storage(); reset_metadata(); on the page that's fully invalidated? (Perhaps we can make that another template parameter, if needed, and enable it by default.)

(Only in debug development situations should we just accumulate memory and never free it, IMHO.)

Generally, I agree, the memory should be recycled in such cases. Although, technically, I have another proposition for how to implement this.

So, to keep the internal housekeeping as simple, as it was before, I think instead of re-initializing existing pages, better to put them into "deactivated" (or "discarded") state indefinitely (that is, free their internal storage) and just allocate new ones at the end.

There's even more to why I think we should just abandon older pages:

Theoretically, we can run into a situation which involves undesired iteration behavior.

Say, we have an iterator I pointing to element E1 at {X, Y} page index (which points into page Z). Suppose, after a while, page Z gets invalidated and later re-initialized, giving us another MaxElementsPerPage fresh indices in the container c.

After some more time, we allocate a new element E2 at {X, Y} page index and access the iterator I, which will access the E2 instead of E1. And this can be very wrong and dangerous for a bunch of reasons. So, this actually breaks the guarantees, which ReuseSlots=false gives us in terms of pointer safety.

I'll prepare a patch to implement this.

@past-due
Copy link
Member

past-due commented Mar 20, 2024

Generally, I agree, the memory should be recycled in such cases. Although, technically, I have another proposition for how to implement this.

So, to keep the internal housekeeping as simple, as it was before, I think instead of re-initializing existing pages, better to put them into "deactivated" (or "discarded") state indefinitely (that is, free their internal storage) and just allocate new ones at the end.

Great idea for ReuseSlots=false mode. 👍

…false`

This helps to keep memory consumption under control.

Existing expired pages are left in the deactivated
state indefinitely instead of reallocating their storage
upon expiration, so that we don't break the indexing
and memory safety guarantees for the container.

Signed-off-by: Pavel Solodovnikov <[email protected]>
@past-due past-due merged commit cc0e573 into Warzone2100:master Mar 21, 2024
38 checks passed
@ManManson ManManson deleted the droids_optimized_storage_alt branch March 22, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants