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

add support for preloading the icon_exists cache #5530

Merged
merged 10 commits into from
Feb 22, 2025

Conversation

Absolucy
Copy link
Member

@Absolucy Absolucy commented Feb 18, 2025

About The Pull Request

uses https://github.com/absolucy/icon-presence-cache

Why It's Good For The Game

better performance

Changelog

no player-facing changes

@Absolucy Absolucy added do not merge don't merge this ffs Performance / Optimization the number going down makes me happy :3 labels Feb 18, 2025
@LikeLakers2
Copy link
Collaborator

LikeLakers2 commented Feb 18, 2025

As an aside - I might suggest putting this preloading functionality behind a compile-time flag... let's say PRELOAD_ICON_EXISTS_CACHE.

Then, if this flag is specified, we call into a new implementation of icon_exists() that uses the preloading - otherwise, we call into the old implementation:

#ifdef PRELOAD_ICON_EXISTS_CACHE

/proc/icon_exists(file, state)
	var/static/list/icon_states_cache = load_icon_exists_cache()
	var/file_name = "[file]"
	return !(isnull(icon_states_cache[file_name]?[state])

#else

/proc/icon_exists(file, state)
	// original implementation goes here

#endif

This avoids running code specific to the preloaded cache version, if we aren't using it (or otherwise don't want it) - and simultaneously, speeds up the preloaded cache version by not trying to populate the cache should the file not exist.

This, in turn, keeps icon_exists() as fast as possible in all cases - which is important, as icon_exists is a bit of a hot proc.

@Absolucy Absolucy marked this pull request as draft February 18, 2025 13:10
@Absolucy Absolucy added the do not testmerge 50-50 chance this gets testmerged anyways label Feb 18, 2025
@Absolucy Absolucy removed the do not testmerge 50-50 chance this gets testmerged anyways label Feb 18, 2025
@Absolucy Absolucy marked this pull request as ready for review February 18, 2025 18:26
@Absolucy Absolucy added the Tools label Feb 18, 2025
// #define DISABLE_DREAMLUAU

/// If this is uncommented, /proc/icon_exists will attempt to load an initial cache from icon_exists_cache.json
// #define PRELOAD_ICON_EXISTS_CACHE
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I wouldn't mind this being uncommented by default, if you think it'd be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

currently there's not even a build step to do this, i'm doing this manually / via TGS build scripts

@Veth-s Veth-s added the admin approved admins say it's fine, just awaiting code review label Feb 21, 2025
@Absolucy Absolucy removed the do not merge don't merge this ffs label Feb 21, 2025
@Absolucy Absolucy merged commit 21c2b18 into Monkestation:master Feb 22, 2025
25 checks passed
@Absolucy Absolucy deleted the icon-exists-cache branch February 22, 2025 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin approved admins say it's fine, just awaiting code review Performance / Optimization the number going down makes me happy :3 Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants