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

Performance regression in "Items" step of the Verifying process in loading a game #71371

Open
NetSysFire opened this issue Jan 30, 2024 · 10 comments · Fixed by #72064
Open

Performance regression in "Items" step of the Verifying process in loading a game #71371

NetSysFire opened this issue Jan 30, 2024 · 10 comments · Fixed by #72064
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Performance Performance boosting code (CPU, memory, etc.) (P2 - High) High priority (for ex. important bugfixes) (S2 - Confirmed) Bug that's been confirmed to exist

Comments

@NetSysFire
Copy link
Member

Describe the bug

Compared to some months ago (around November), this step takes significantly longer and I can not find the cause for it.

Attach save file

n/a

Steps to reproduce

  1. Load or create any game.
  2. Notice that said step is taking very long.

image

Expected behavior

It does not take that long.

On the devcord a rando confirmed that they noticed the same, but its more of an anecdote than anything, so I will not be marking this as confirmed on creation.
I also tried to "bisect" via using older releases but for the love of $deity I can not really find known good and bad candidates except a vague "one from November somewhere" because I did not record which build I was using prior to updating.

I did crudely measure the time it takes to load a game with /usr/bin/time (just exited as soon as loading was done):

11.39s user 0.54s system 80% cpu 14.853 total

vs

14.90s user 0.67s system 86% cpu 18.073 total

But it feels significantly longer than that.

Screenshots

No response

Versions and configuration

  • OS: Linux
    • OS Version: LSB Version: n/a; Distributor ID: Arch; Description: Arch Linux; Release: rolling; Codename: n/a;
  • Game Version: dbd4067 [64-bit]
  • Graphics Version: Tiles
  • Game Language: System language []
  • Mods loaded: [
    Dark Days Ahead [dda],
    Disable NPC Needs [no_npc_food],
    Portal Storms Ignore NPCs [personal_portal_storms],
    Slowdown Fungal Growth [no_fungal_growth]
    ]

Additional context

No response

@NetSysFire NetSysFire added Code: Performance Performance boosting code (CPU, memory, etc.) (S1 - Need confirmation) Report waiting on confirmation of reproducibility (S2 - Confirmed) Bug that's been confirmed to exist [C++] Changes (can be) made in C++. Previously named `Code` and removed (S1 - Need confirmation) Report waiting on confirmation of reproducibility labels Jan 30, 2024
@ZhilkinSerg
Copy link
Contributor

image

@ZhilkinSerg
Copy link
Contributor

Item consistency just spawns a lot of items, so it adds up to a significant time. This check can be moved to a test suite or hidden behind some option to speed up loading times or maybe check results can be cached somehow.

@RenechCDDA
Copy link
Member

@akrieger bisected this last night and surprisingly the major culprit appears to have been a json change. Link to discussion: https://discord.com/channels/598523535169945603/598535827169083403/1202085328475922432

"the first major regression from 9ish seconds to 22/25ish seconds was #70370"

image

(a follow-up this morning, the day after): "if i do this on master then item verification drops to only about 6 seconds"

@turco-ai
Copy link

turco-ai commented Feb 4, 2024

I can confirm that the game would get stuck on items for quite a while and I thought that was normal because more stuff got added, I'm on Android so I feel every performance improvement.(and deprovements if you will)

@NetSysFire NetSysFire pinned this issue Feb 4, 2024
@NetSysFire NetSysFire added the (P2 - High) High priority (for ex. important bugfixes) label Feb 4, 2024
@Inglonias
Copy link
Contributor

@akrieger bisected this last night and surprisingly the major culprit appears to have been a json change. Link to discussion: https://discord.com/channels/598523535169945603/598535827169083403/1202085328475922432

I wonder if this is happening because of the sheer size of the item stacks being counted here? That's all I can think of, anyway.

@SurFlurer
Copy link
Contributor

diff --git a/data/json/itemgroups/supplies.json b/data/json/itemgroups/supplies.json
index 151eec6439..5b6909769d 100644
--- a/data/json/itemgroups/supplies.json
+++ b/data/json/itemgroups/supplies.json
@@ -56,25 +56,25 @@
     "id": "sandbag",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_sand", "count": 2500 } ]
+    "items": [ { "item": "material_sand", "charges": 2500 } ]
   },
   {
     "id": "sandbag_part",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_sand", "count": [ 400, 2500 ] } ]
+    "items": [ { "item": "material_sand", "charges": [ 400, 2500 ] } ]
   },
   {
     "id": "gravelbag",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_gravel", "count": 2500 } ]
+    "items": [ { "item": "material_gravel", "charges": 2500 } ]
   },
   {
     "id": "gravelbag_part",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_gravel", "count": [ 400, 2500 ] } ]
+    "items": [ { "item": "material_gravel", "charges": [ 400, 2500 ] } ]
   },
   {
     "id": "earthbag",
@@ -104,25 +104,25 @@
     "id": "cement_bag",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_cement", "count": 2800 } ]
+    "items": [ { "item": "material_cement", "charges": 2800 } ]
   },
   {
     "id": "cement_bag_part",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_cement", "count": [ 400, 2800 ] } ]
+    "items": [ { "item": "material_cement", "charges": [ 400, 2800 ] } ]
   },
   {
     "id": "quicklime_bag",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_quicklime", "count": 1200 } ]
+    "items": [ { "item": "material_quicklime", "charges": 1200 } ]
   },
   {
     "id": "quicklime_bag_part",
     "type": "item_group",
     "container-item": "bag_durasack",
-    "items": [ { "item": "material_quicklime", "count": [ 400, 1200 ] } ]
+    "items": [ { "item": "material_quicklime", "charges": [ 400, 1200 ] } ]
   },
   {
     "id": "paintcans",

Although using charges, this change does cut the time taken by"Items" step by 60%.

@Zireael07
Copy link
Contributor

I think this is the confirmation that the move to get rid of charges will bring about perf trouble

@NetSysFire
Copy link
Member Author

NetSysFire commented May 6, 2024

I am afraid this is happening again since a week or two or so.
Edit: This should not affect the 0.H release since the cause will probably not be backported.

@NetSysFire NetSysFire reopened this May 6, 2024
@NetSysFire
Copy link
Member Author

perf.data.gz

Due to using my own build, I managed to gather perf data since I had debugging symbols there.

@Maleclypse
Copy link
Member

Maleclypse commented Aug 7, 2024

You’ll want to move it back to potential blockers.

Edit: i remembered where the button was on mobile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Performance Performance boosting code (CPU, memory, etc.) (P2 - High) High priority (for ex. important bugfixes) (S2 - Confirmed) Bug that's been confirmed to exist
Projects
Status: Postponed/Not Blocker
Development

Successfully merging a pull request may close this issue.

8 participants