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

Cache item info for crafting GUI full-text search #77914

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

Brambor
Copy link
Contributor

@Brambor Brambor commented Nov 16, 2024

Summary

Performance "Cache item info for crafting GUI full-text search"

Purpose of change

Getting info on an item item.info( true ) made more than 95% of the searching time (1m11s on my PC). When cached (rerun), the search takes about 400 ms! Making it over 100x faster.

Speeds up on multiple levels:

  1. If you cancel the full-text search, it still cached the data it went through.
    • Full-text search for something. Realize you made a mistake while it searches. ESC, fix your prompt and do a full-text search again. Essentially all the time from the previous (cancelled) search is reused!
  2. If you have multiple full-text searches in a prompt, the first caches all the data and the next ones use that cache.
  3. Refining your search prompt is super fast now. Try one (takes a minute), see results, try another one (takes like 400ms).
  4. You can search; close the crafting GUI to look at what you are wearing etc (as long as you don't take a turn); reopen the crafting GUI; the cache still works, so search it again!
  5. I noticed switching a crafter when a search prompt was used, it re-searches it. With this, it takes no time at all! Btw. all searches are avatar character-centric, so they are agnostic to the crafter Character.

Describe the solution

  • Cache the items in src/uistate.h. Should the code live somewhere else? a variable local to src/recipe_dictionary.cpp.
  • In full-text search, use the cache function instead of the regular function.
  • Cache only for this turn.

Describe alternatives you've considered

Making the regular function cache automatically. It would cause more changes to test.

The item info probably breaks (is not up to date) when you do something in less than a turn. I considered fixing that, but first I would need to find an example. Maybe it will matter more if it is used someplace else, like crafting GUI in general. And if it gets used.

Testing

Before

  1. Searched before this PR. It takes ages: d:close takes 1min8sec.

After

  1. Searched now. It takes ages. Actually, it takes less time d:close takes 56sec now. The cache is utilized even for the first full-text search apparently. It has 845 hits and 3318 misses with a clear cache. The second time, it has all hits and no misses.
  2. Cancel the search and redo it - it picks up (caching) where it left off.
  3. Same if you let the search finish.
  4. Multiple full-text searches, like the example d:close to skin,d:HEAD. Took 55 seconds. Nice bonus: the percentage progress now concludes at 100% and finishes, before, it made the second 0 to 100% search for the second full-text search.
  5. Search, close crafting GUI, open it, and search. The second search is fast.
  6. Switching crafter with a full-text prompt is fast now. (And still respects item inventories etc. It seems to work!)
  7. Waiting for a turn invalidates the cache.

Am I respecting item variants? Or something? Does it break for something?

Additional context

This could be used more. Like connected to the crafting GUI. I wanted to start with a super simple and useful implementation. We can do the rest in subsequent PRs.

The text might need to be wrapped for other use cases. In searching there is no wrapping.

@Brambor Brambor added the Code: Performance Performance boosting code (CPU, memory, etc.) label Nov 16, 2024
@github-actions github-actions bot added <Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling [C++] Changes (can be) made in C++. Previously named `Code` labels Nov 16, 2024
@Brambor Brambor added <Bugfix> This is a fix for a bug (or closes open issue) and removed <Bugfix> This is a fix for a bug (or closes open issue) labels Nov 16, 2024
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Nov 16, 2024
src/uistate.h Outdated Show resolved Hide resolved
@Brambor Brambor force-pushed the search-cache-item-info branch from 277ba09 to 5e0c847 Compare November 16, 2024 22:21
@Brambor Brambor force-pushed the search-cache-item-info branch from 5e0c847 to 38955fc Compare November 16, 2024 22:40
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 16, 2024
@Brambor
Copy link
Contributor Author

Brambor commented Nov 17, 2024

Test failure unrelated:

(~[slow] ~[.],starting_items)=> (continued from above) ERROR : src/newcharacter.cpp:685 [void Character::add_profession_items()] Failed to place 3 items in inventory for profession Crossbow Hunter

https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/11873797927/job/33089014493?pr=77914

Restarting tests.

@Brambor Brambor closed this Nov 17, 2024
@Brambor Brambor reopened this Nov 17, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 17, 2024
@Night-Pryanik Night-Pryanik merged commit 94048ee into CleverRaven:master Nov 18, 2024
37 of 52 checks passed
@Brambor Brambor deleted the search-cache-item-info branch November 26, 2024 23:18
@Brambor
Copy link
Contributor Author

Brambor commented Dec 26, 2024

The cache survives the save-load cycle.

Potential bugs:

World bug
Make worlds A and B. Get them to the same turn. Example: copy-paste a world.

Fulltext searching items in world A will cache the results. Load world B. It will use this cache.

It is unlikely that anyone will ever encounter this bug.

Avatar bug
Use full-text search to cache info.

Switch conciseness from avatar A to another character B (I assume this does not take a turn, if it does, the cache is correctly invalidated). Also, I assume this can be done by choosing a new leader. Debug menu-only bugs solved by waiting a turn are not worth fixing.

Use cached info with avatar B.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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` Code: Performance Performance boosting code (CPU, memory, etc.) Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants