Skip to content

Minimise allocations for ModifierSum#755

Merged
wvpm merged 1 commit into
masterfrom
dev_minimise_allocations_modifiersum
May 25, 2026
Merged

Minimise allocations for ModifierSum#755
wvpm merged 1 commit into
masterfrom
dev_minimise_allocations_modifiersum

Conversation

@wvpm

@wvpm wvpm commented May 23, 2026

Copy link
Copy Markdown
Contributor

Original PR: #739

CountryInstance::contribute_province_modifier_sum is called for each province (expect 10s-100s per country). Every call to ModifierSum::add_modifier_sum called reserve_more, which is defined as:

    constexpr void reserve_more(reservable auto& t, size_t size) {
        t.reserve(t.size() + size);
    }

This resulted in repeated allocations, possibly reallocating the country's modifiersum for each controlled province.
Instead we now first sum the extra size for all provinces and then resize (not reserve!) once.
We use resize instead of reserve as it uses the underlying growth algorithm that typically reserves extra room, avoiding reallocating because of 1 extra element.

Resize creates empty elements which are later replaced. The ModifierSum keeps track of the count of valid elements and uses this for its size().

@wvpm
wvpm requested a review from a team as a code owner May 23, 2026 09:16
@wvpm wvpm added topic:codestyle Issue that applies stylistic changes to the module, semantics and mechanics should not change performance Issue directly concerned with performance or that is specifically intended to improve performance topic:society Issue that relates specifically to simulation societal function, like religion, culture, politics labels May 23, 2026
@wvpm
wvpm enabled auto-merge May 23, 2026 09:18
@Spartan322

Copy link
Copy Markdown
Member

First off squash the merge away, secondly I still firmly hold on not merging bulk_insert_wrapper as I think its completely unnecessary compared to #746 and maybe tracking inside ModifierSum if you really want to handle mult-threaded syncing.

@Spartan322 Spartan322 changed the title [from dev] Minimise allocations for ModifierSum Minimise allocations for ModifierSum May 23, 2026
@wvpm
wvpm force-pushed the dev_minimise_allocations_modifiersum branch from a152678 to 8ef822c Compare May 24, 2026 14:42
@wvpm

wvpm commented May 24, 2026

Copy link
Copy Markdown
Contributor Author

Once #746 becomes available, I plan to do a follow up PR that makes bulk_insert_wrapper use tunable_vector internally.

@wvpm
wvpm force-pushed the dev_minimise_allocations_modifiersum branch from 8ef822c to f86728d Compare May 25, 2026 12:47

@Catylist0 Catylist0 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved as discussed in discord

@wvpm
wvpm disabled auto-merge May 25, 2026 14:40
@wvpm
wvpm merged commit 40a8de8 into master May 25, 2026
16 checks passed
@wvpm
wvpm deleted the dev_minimise_allocations_modifiersum branch May 25, 2026 14:40

@Catylist0 Catylist0 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixing bugged review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Issue directly concerned with performance or that is specifically intended to improve performance topic:codestyle Issue that applies stylistic changes to the module, semantics and mechanics should not change topic:society Issue that relates specifically to simulation societal function, like religion, culture, politics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants