Compute can_holser once per save to speed up `d:` filter from ~20s to ~5s #78950

Brambor · 2025-01-05T01:13:34Z

Summary

Performance "Compute can_holser once per save to speed up d: filter from ~20s to ~5s"

Purpose of change

Speed up d: filter from 20s to 5s (all searches after the first search)
- The speed up from Cache item info, reducing time of crafting filter d: to ~32% #78945 is included in the 20s.
Speed up info for item (untested): short string info generates too long (725 ms) #78749
- My last step towards it, I will likely test it a bit and then close it.

It is not the purpose, but this PR changes the "Can be stored in" item info:

TODO: recheck after fixing

Before this PR, it checks volume etc. of this item and finds anything this can fit in.
- Can be stored for half filled camera bag respects the contents of camera bag.
After this PR, only the type is checked, so it lists all items that this item would fit into, if this item was empty. Honestly, I think this is an improvement.
- Can be stored for half-filled camera bag behaves as if camera bag was empty.

Honestly, I think this is an improvement. I expect this menu to be "What could this item fit into" rather than "What does this item fit into right now."

Also, I don't use it and it consumes so much resources for this and item_info. This is a way to keep it and have it fast.

Describe the solution

Make a new cache for can_contain for holsters in item_factory. The cache stores all items itype fits into. Since itypes are created on loading a save, we can start caching at that point. Since itypes are frozen, the cache is valid until the save is Quit. (See additional context for m_runtimes).

To be done:

Make it work again for non-empty items.
- By (always) iterating over the given vector of containers and eliminating those we don't fit in)
Make it work again for items smaller than itype suggests.
- I need to find the optimal solution. Probably figure out the gun is foldable. I hope this will lead me to a universal solution.
- If item < type, don't use the cache. Unless a better option is found.
In light of "make it work again":
- Document where the cache doesn't work.
- Make a new method item::contained_in (name pending) using this cache but making it work for the above two problems.

Describe alternatives you've considered

To prevent the change described in purpose:

It can be changed though (add a parameter to final_info to use this cache; pass that parameter only from the crafting menu)

Testing

Open the crafting menu and search using d: filter - takes about 20 seconds.
Wait a turn to invalidate the search cache. (But not the holster cache!)
Search again, takes about 5 seconds from now on.
Save and Quit. (Do not turn off the game).
Load.
It Takes 20 the first time again, indicating the holster cache cleared.

Profiling

First search

Second+ search

Additional context

I do not understand what m_runtimes in src/item_factory.h are, but I assume they are camp-related.

We discussed it here https://discord.com/channels/598523535169945603/598529174302490644/1325252546281078814

As long as they are not actual items that can contain things, this PR doesn't care. If they are, this should invalidate the cache whenever m_runtimes changes.

item_factory::add_item_type is dead code, see remove dead code #79004.

src/item_factory.h

CLIDragon · 2025-01-05T01:41:27Z

src/item_factory.h

@@ -285,6 +292,9 @@ class Item_factory
        std::unordered_map<itype_id, ammotype> migrated_ammo;
        std::unordered_map<itype_id, itype_id> migrated_magazines;

+        // cache for holsters
+        std::unordered_map<itype_id, std::vector<const itype *>> type_contained_in_holsters;


std::vector<std::reference_wrapper<const itype>> would probably be a better fit than a std::vector<const itype *>>.

Actually, Item_factory::find returns std::vector<const itype *>. So I would reinterpret it here as a vector of references. (No big deal)

It seems that whenever working with vector of itype, we use pointers. It might be that the code is old.

Do you still think I should change it to refs?

It seems that whenever working with vector of itype, we use pointers. It might be that the code is old.

I think it is just that the code is old, and everyone who has used the code since then has just kept it as is. There isn't any performance cost to converting a pointer to a reference, so I think they should be stored as references.

CLIDragon · 2025-01-05T01:43:17Z

src/item_factory.cpp

+        const holster_actor *ptr = dynamic_cast<const holster_actor *>
+                                   ( e.get_use( "holster" )->get_actor_ptr() );
+        const item holster_item( &e );
+        return ptr->can_holster( holster_item, itm ) && !item_is_blacklisted( holster_item.typeId() );


Is the cache updated on blacklist changes?

It is not. I guess we don't need this at all. Thanks for bringing this up whether it concludes to be not needed or completely wrong.

I removed the check.

Given the check exists in the old code, I would be wary that removing it removes functionality. That said, I don't really use blacklisting when I play, so I don't know what it would cause.

I think we could refresh the cache on blacklistings... If that is wanted.

moxian · 2025-01-05T02:07:36Z

Honestly, I think this is an improvement. I expect this menu to be "What could this item fit into" rather than "What does this item fit into right now."

If you think this can be confusing, consider rewording Can be stored in: -> Can be stored in (while empty):. Perhaps only conditionally, if the item has non-rigid pockets.
(Personally I don't have an opinion here, and either wording is fine for my tastes)

- Second search with `d:` in crafting menu takes ~5 seconds instead of ~20 seconds

Brambor · 2025-01-05T02:34:44Z

Could be. I don't like the "Can be stored in", it is probably broken. It doesn't list enough things, it probably only lists clothing.

Compare Can be stored in the description on a plank and then search crafting menu for L:122cm. Some of those containers should be shown in "can be stored in", in my opinion.

I am writing that because I don't know what the original intent was so I don't want to change the sentence without investigating what the list actually represents.

That said, this PR doesn't care what it lists, it just caches it.

Brambor · 2025-01-05T04:45:09Z

Oh, I know now. Let's use the cache as a first line of defence, giving candidates. Then check these candidates. It will still reduce the number of items to check from like 8000 to between 0 and 40. A bit slower. But there will be no change in functionality.

Gonna sleep now.

andrei8l · 2025-01-05T13:27:01Z

This isn't correct. Some items can change volume/weight/length so they won't fit in your precomputed holsters anymore. A quick example is a Glock carry pistol that fits unmodified in a fast-draw holster, but not when modded with a pistol suppressor. All items with non-rigid pockets are also affected.

Brambor · 2025-01-05T13:48:34Z

You are commenting the existing code, not the last idea, right?

With the last idea #78950 (comment), we would get candidates that need checking.

I asked on Discord and what breaks the last idea are items, that can get smaller, not bigger. They then could fit into more containers than precomputed.

Brambor · 2025-01-05T14:02:18Z

Or can't we just tell if item is modified? If it is, don't use this cache.

GuardianDll · 2025-01-07T06:51:48Z

Is this ready to be merged?

CLIDragon · 2025-01-07T09:15:20Z

I don't believe so, as it currently incorrectly handles items that can be modified to be smaller. The gun is a clear example, but the police baton or collapsing mop would also be a problem.

Brambor · 2025-01-07T11:24:33Z

I am converting this to a draft to clarify that it is not ready (in addition to the unresolved conversations).

I updated the OP on what needs to be done.

github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Code: Performance Performance boosting code (CPU, memory, etc.) labels Jan 5, 2025

Brambor changed the title ~~Compute can_holser once per save to speed up d: filter from 20s to 5s~~ Compute can_holser once per save to speed up d: filter from ~20s to ~5s Jan 5, 2025

github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 5, 2025

CLIDragon suggested changes Jan 5, 2025

View reviewed changes

cache holsters per loaded save

ca5c6db

- Second search with `d:` in crafting menu takes ~5 seconds instead of ~20 seconds

Brambor force-pushed the cache-holster branch from 332208e to ca5c6db Compare January 5, 2025 02:15

github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 5, 2025

Brambor marked this pull request as draft January 7, 2025 11:23

Brambor mentioned this pull request Jan 7, 2025

remove dead code #79004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute can_holser once per save to speed up `d:` filter from ~20s to ~5s #78950

Compute can_holser once per save to speed up `d:` filter from ~20s to ~5s #78950

Brambor commented Jan 5, 2025 •

edited

Loading

CLIDragon Jan 5, 2025

Brambor Jan 5, 2025 •

edited

Loading

CLIDragon Jan 5, 2025

CLIDragon Jan 5, 2025

Brambor Jan 5, 2025

Brambor Jan 5, 2025

CLIDragon Jan 5, 2025

Brambor Jan 5, 2025

moxian commented Jan 5, 2025

Brambor commented Jan 5, 2025

Brambor commented Jan 5, 2025

andrei8l commented Jan 5, 2025

Brambor commented Jan 5, 2025

Brambor commented Jan 5, 2025

GuardianDll commented Jan 7, 2025

CLIDragon commented Jan 7, 2025

Brambor commented Jan 7, 2025

Compute can_holser once per save to speed up d: filter from ~20s to ~5s #78950

Are you sure you want to change the base?

Compute can_holser once per save to speed up d: filter from ~20s to ~5s #78950

Conversation

Brambor commented Jan 5, 2025 • edited Loading

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

CLIDragon Jan 5, 2025

Choose a reason for hiding this comment

Brambor Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

CLIDragon Jan 5, 2025

Choose a reason for hiding this comment

CLIDragon Jan 5, 2025

Choose a reason for hiding this comment

Brambor Jan 5, 2025

Choose a reason for hiding this comment

Brambor Jan 5, 2025

Choose a reason for hiding this comment

CLIDragon Jan 5, 2025

Choose a reason for hiding this comment

Brambor Jan 5, 2025

Choose a reason for hiding this comment

moxian commented Jan 5, 2025

Brambor commented Jan 5, 2025

Brambor commented Jan 5, 2025

andrei8l commented Jan 5, 2025

Brambor commented Jan 5, 2025

Brambor commented Jan 5, 2025

GuardianDll commented Jan 7, 2025

CLIDragon commented Jan 7, 2025

Brambor commented Jan 7, 2025

Compute can_holser once per save to speed up `d:` filter from ~20s to ~5s #78950

Compute can_holser once per save to speed up `d:` filter from ~20s to ~5s #78950

Brambor commented Jan 5, 2025 •

edited

Loading

Brambor Jan 5, 2025 •

edited

Loading