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

GAGS is far too expensive #9

Open
Mothblocks opened this issue Dec 2, 2022 · 5 comments
Open

GAGS is far too expensive #9

Mothblocks opened this issue Dec 2, 2022 · 5 comments
Assignees

Comments

@Mothblocks
Copy link
Member

GAGS has to perform very expensive icon operations for every color variation that is needed. This creates a very nice interface, where you can just slot in icon, but it carries a huge weight on init times.

Worse, the cost of GAGS makes profiling the cost of anything related to GAGS harder to measure. More on this later.

Estimated Cost

/datum/controller/subsystem/processing/greyscale/proc/GetColoredIconByType is 1.46 seconds of init time.

Howver, 91% of calls are only 22ms. The rest are fairly scattered at the end. This is most likely the cache.

The bulk cost is simply in icon operations. GetPixel (1.19s cost, from /datum/greyscale_config/proc/GenerateBundle) especially is very slow, but because it is serving its purpose of forcing the icon to generate--any icon operation would be doing the same.

Solutions

It is possible to precompile statically-inferrable GAGS usages. As in, everything we can predict is going to use GAGS (like from constant greyscale_config and greyscale_colors values) can be put into a .dmi, pushing the costs away from init time. This would come with an extra compile time cost, but it could only be incurred when the inputs are different, like if a .dmi changes or a variable changes. Dynamic generation still exists, but we statically create as much as we can. There is a prototype of this in Rust, but we could even potentially have a DM backend for this and run dd.exe when it comes out to use it.

It is also potentially possible for objects to choose between generating icons and using something like overlays, when applicable. However this could carry non-negligible runtime costs from either adding the overlays, or maptick costs if vis_contents was used. It also is under the assumption the bulk of this cost is in these simple icons, which we have not yet proven.

@Mothblocks
Copy link
Member Author

diff --git a/code/controllers/subsystem/processing/greyscale.dm b/code/controllers/subsystem/processing/greyscale.dm
index 0c0db7b4f70..ec2b7a5a537 100644
--- a/code/controllers/subsystem/processing/greyscale.dm
+++ b/code/controllers/subsystem/processing/greyscale.dm
@@ -34,6 +34,8 @@ PROCESSING_SUBSYSTEM_DEF(greyscale)
 		configurations[i].Refresh(TRUE)
 
 /datum/controller/subsystem/processing/greyscale/proc/GetColoredIconByType(type, list/colors)
+	if (UNLINT(TRUE))
+		return 'icons/testing/greyscale_error.dmi'
 	if(!ispath(type, /datum/greyscale_config))
 		CRASH("An invalid greyscale configuration was given to `GetColoredIconByType()`: [type]")
 	type = "[type]"

@optimumtact
Copy link
Member

if you're going to prebuild gags stuff, don't guess, just change the framework such that you can statically infer everything

@Mothblocks
Copy link
Member Author

  1. That's not possible because dynamic GAGS is something that is actively supported player-facing--we allow custom clothes and stuff like greyscale HUDs want to exist and be configurable
  2. That puts a lot more pressure on the system to be correct--right now a theoretical system could only statically infer half the consumers, and the rest would be a performance sink instead of broken

@optimumtact
Copy link
Member

make it possible

@itsmeow
Copy link

itsmeow commented Jul 15, 2024

BeeStation/rust-g#13
BeeStation/BeeStation-Hornet#10455

Reduces worst-case GAGS generation time from 250ms to 1ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

3 participants