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

Less repeatedly spawning item groups when checking consistency #72064

Merged
merged 3 commits into from
Mar 2, 2024

Conversation

SurFlurer
Copy link
Contributor

@SurFlurer SurFlurer commented Feb 29, 2024

Summary

Performance "Less repeatedly spawning item groups when checking consistency"

Purpose of change

Fix #71371

I noticed that the flame graph of item_group::check_consistency can be divided into two blocks that are similar in shapes. That makes me guess that some groups are spawned multiple times. In fact there's too much unnecessary spawning. We actually do not need to spawn everything over and over if there's no container to restrain them.

Describe the solution

Introduce a bool parameter actually_spawn to the callstack of check_consistency(). The logic below stands on the point that spawning is different from checking consistency. Every item groups and single item creators get their consistency checked, so there's unlikely to be omissions.

  1. When checking an item group, if it doesn't define a container, we do not need to spawn it as a whole, just get individual entries spawned is enough.
  2. When checking a single item creator, if it points to an item group and doesn't define a container, we do not need to spawn it, because the item group will be spawned separately.

Describe alternatives you've considered

collection type item groups automatically spawn all entries when checking consistency, so unless the item group's type is distribution, there's no need to spawn all entries individually. ( this is what the first commit does )

Gains from this alternative:

image

Not as seductive as the the main solution, but I think it's less likely to introduce problems.

Testing

It reduces the time spent on verifying items from 12ish seconds to 2ish seconds.

before:

( note the item_group::check_consistency line )

image

after:

image

I also randomly fiddled with some itemgroups to ensures that their errors can still be detected by new code.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Feb 29, 2024
@SurFlurer SurFlurer marked this pull request as draft March 2, 2024 06:47
@SurFlurer SurFlurer marked this pull request as ready for review March 2, 2024 07:06
@github-actions github-actions bot added the <Bugfix> This is a fix for a bug (or closes open issue) label Mar 2, 2024
@Maleclypse Maleclypse merged commit 669a9fc into CleverRaven:master Mar 2, 2024
22 of 27 checks passed
@Inglonias
Copy link
Contributor

Bravo! Good job.

@SurFlurer SurFlurer deleted the check_consistency branch March 3, 2024 01:06
@Procyonae Procyonae added the 0.H Backport PR to backport to the 0.H stable release canddiate label May 20, 2024
Procyonae pushed a commit to Procyonae/Cataclysm-DDA that referenced this pull request May 20, 2024
…rRaven#72064)

* Less repeatedly spawning item groups when checking consistency

* Further tweaks
@Procyonae Procyonae mentioned this pull request May 20, 2024
Maleclypse added a commit that referenced this pull request May 21, 2024
@Procyonae Procyonae added 0.H Backported and removed 0.H Backport PR to backport to the 0.H stable release canddiate labels May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.H Backported astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance regression in "Items" step of the Verifying process in loading a game
4 participants