From 782a04859ab34752fa4e4c3c4df64d2fcb064da7 Mon Sep 17 00:00:00 2001 From: SkyratBot <59378654+SkyratBot@users.noreply.github.com> Date: Sat, 20 Jan 2024 23:39:52 +0100 Subject: [PATCH] [MIRROR] Macro Optimizes Map Saving (100x) DO NOT CHURN STRINGS Edition (#26189) * Macro Optimizes Map Saving (100x) DO NOT CHURN STRINGS Edition (#80845) ## About The Pull Request Yello! This one is reasonably quick, tho I did some fixes too This is the big one, fixes the buildmode tool sometimes locking disabled for the whole round. We do this by replacing the static var on buildmode with global var and a global proc This keeps a harddel on the buildmode datum from permalocking is_running to TRUE Also makes flipping the var BACK if something breaks significantly easier for admins, so that's nice Alright, smaller things now Fixes lists of numbers failing to encoded improperly This was fixed on shiptest, we failed to actually port their most recent revision Fixes the shuttle flag not actually working because it used istype instead of ispath Changes obj_blacklist to a typecache for optimization's sake Renames/moves some vars around to prevent weird double typing things Removes a checktick in key gen, it's just costing more time then it would save in overtime Properly handles lists. We were only doing var encoding one layer deep, need to do it alll the way down Alright, now the optimizations This proc is fucking HOT, and it's for really dumb reasons This is a text gen proc, and it makes the mistake of generating text and concatinating it with MORE text. This is HORRIFICALLY EXPENSIVE because byond caches strings (can only be one of each) and string churn fucks up that caching system something fierce Moving from strings to lists of strings we join at the end takes us from like idk 100 seconds to save bare metastation to like 1.5 This is applied basically everywhere for obvious reasons While I'm here, storing keys in a flat list and then using find to find them, then using that index to lookup into another flat list is a bit silly. Let's just make it an assoc list. Faster lookup, cleaner. Oh also rather then iterating over all the vars on an object, let's iterate over just the ones we care about yeah? Let's see... no sense genning a key we'll never use, and having suffixes be often non existent is silly just embrace the slight mess. That's it I think, this takes us from 100 seconds to save metastation to 2.5 seconds to save ALL of metastation (I removed the vars limiter so I could make sure var saving didn't fuck me up) ## Why It's Good For The Game Cleans up some issues that we failed to port the fixes for, MASSIVELY optimizes this (so it can finish in like 5/10 seconds and not 300!) and ensures admins can always use the thing and don't risk dropping their pet buildastation to the void. Worth noting, this tool really should not be used for station mapping outside an event context. It produces sorta buggy var edits, and WILL fail to pull over context for shit. Please don't use it as such Profiles (csv files I promise) [Before](https://github.com/tgstation/tgstation/files/13853313/profiler.json) [After](https://github.com/tgstation/tgstation/files/13853271/profiler.json) I'd include my line by lines but I don't know how much you'd get out of them. Here's an image tho ![image](https://github.com/tgstation/tgstation/assets/58055496/3f3148c5-8b1e-4bda-aa65-3983f9944a91) ## Changelog :cl: fix: The map saving tool will no longer lock up and prevent all further action at random fix: Map saving now takes on the order of seconds, not minutes fix: Fixes an issue with lists that caused strongdmm to report saved maps as broken /:cl: * Macro Optimizes Map Saving (100x) DO NOT CHURN STRINGS Edition --------- Co-authored-by: LemonInTheDark <58055496+LemonInTheDark@users.noreply.github.com> --- code/modules/admin/verbs/map_export.dm | 164 +++++++++--------- code/modules/buildmode/submodes/map_export.dm | 33 ++-- code/modules/mapping/reader.dm | 2 - 3 files changed, 107 insertions(+), 92 deletions(-) diff --git a/code/modules/admin/verbs/map_export.dm b/code/modules/admin/verbs/map_export.dm index 88e80414235..8ff2568e79a 100644 --- a/code/modules/admin/verbs/map_export.dm +++ b/code/modules/admin/verbs/map_export.dm @@ -73,6 +73,7 @@ * This has been made semi-modular so you should be able to use these functions * elsewhere in code if you ever need to get a file in the .dmm format **/ + /atom/proc/get_save_vars() return list( NAMEOF(src, color), @@ -127,18 +128,42 @@ GLOBAL_LIST_INIT(save_file_chars, list( "Y","Z", )) -/proc/to_list_string(list/future_string) - . = "list(" +/proc/to_list_string(list/build_from) + var/list/build_into = list() + build_into += "list(" var/first_entry = TRUE - for(var/item in future_string) + for(var/item in build_from) + CHECK_TICK if(!first_entry) - . += ", " - if(future_string[item]) - . += hashtag_newlines_and_tabs("[item] = [future_string[item]]", list("{"="", "}"="", "\""="", ";"="", ","="")) + build_into += ", " + if(isnum(item) || !build_from[item]) + build_into += "[tgm_encode(item)]" else - . += hashtag_newlines_and_tabs("[item]", list("{"="", "}"="", "\""="", ";"="", ","="")) + build_into += "[tgm_encode(item)] = [tgm_encode(build_from[item])]" first_entry = FALSE - . += ")" + build_into += ")" + return build_into.Join("") + +/// Takes a constant, encodes it into a TGM valid string +/proc/tgm_encode(value) + if(istext(value)) + //Prevent symbols from being because otherwise you can name something + // [";},/obj/item/gun/energy/laser/instakill{name="da epic gun] and spawn yourself an instakill gun. + return "\"[hashtag_newlines_and_tabs("[value]", list("{"="", "}"="", "\""="", ";"="", ","=""))]\"" + if(isnum(value) || ispath(value)) + return "[value]" + if(islist(value)) + return to_list_string(value) + if(isnull(value)) + return "null" + if(isicon(value) || isfile(value)) + return "'[value]'" + // not handled: + // - pops: /obj{name="foo"} + // - new(), newlist(), icon(), matrix(), sound() + + // fallback: string + return tgm_encode("[value]") /** *Procedure for converting a coordinate-selected part of the map into text for the .dmi format @@ -154,7 +179,6 @@ GLOBAL_LIST_INIT(save_file_chars, list( shuttle_area_flag = SAVE_SHUTTLEAREA_DONTCARE, list/obj_blacklist = list(), ) - var/width = maxx - minx var/height = maxy - miny var/depth = maxz - minz @@ -164,56 +188,57 @@ GLOBAL_LIST_INIT(save_file_chars, list( var/layers = FLOOR(log(GLOB.save_file_chars.len, turfs_needed) + 0.999,1) //Step 1: Run through the area and generate file data - var/list/header_chars = list() //The characters of the header - var/list/header_dat = list() //The data of the header, lines up with chars - var/header = "" //The actual header in text - var/contents = "" //The contents in text (bit at the end) - var/index = 1 + var/list/header_data = list() //holds the data of a header -> to its key + var/list/header = list() //The actual header in text + var/list/contents = list() //The contents in text (bit at the end) + var/key_index = 1 // How many keys we've generated so far for(var/z in 0 to depth) for(var/x in 0 to width) contents += "\n([x + 1],1,[z + 1]) = {\"\n" for(var/y in height to 0 step -1) CHECK_TICK //====Get turfs Data==== - var/turf/place = locate((minx + x), (miny + y), (minz + z)) + var/turf/place var/area/location - var/list/objects - var/area/place_area = get_area(place) + var/turf/pull_from = locate((minx + x), (miny + y), (minz + z)) //If there is nothing there, save as a noop (For odd shapes) - if(!place) + if(isnull(pull_from)) place = /turf/template_noop location = /area/template_noop - objects = list() //Ignore things in space, must be a space turf - else if(istype(place, /turf/open/space) && !(save_flag & SAVE_SPACE)) + else if(istype(pull_from, /turf/open/space) && !(save_flag & SAVE_SPACE)) place = /turf/template_noop location = /area/template_noop + pull_from = null //Stuff to add else + var/area/place_area = get_area(pull_from) location = place_area.type - objects = place - place = place.type + place = pull_from.type + //====Saving shuttles only / non shuttles only==== - var/is_shuttle_area = istype(location, /area/shuttle) + var/is_shuttle_area = ispath(location, /area/shuttle) if((is_shuttle_area && shuttle_area_flag == SAVE_SHUTTLEAREA_IGNORE) || (!is_shuttle_area && shuttle_area_flag == SAVE_SHUTTLEAREA_ONLY)) place = /turf/template_noop location = /area/template_noop - objects = list() + pull_from = null //====For toggling not saving areas and turfs==== if(!(save_flag & SAVE_AREAS)) location = /area/template_noop if(!(save_flag & SAVE_TURFS)) place = /turf/template_noop //====Generate Header Character==== - var/header_char = calculate_tgm_header_index(index, layers) //The characters of the header - var/current_header = "(\n" //The actual stuff inside the header + // Info that describes this turf and all its contents + // Unique, will be checked for existing later + var/list/current_header = list() + current_header += "(\n" //Add objects to the header file var/empty = TRUE //====SAVING OBJECTS==== if(save_flag & SAVE_OBJECTS) - for(var/obj/thing in objects) + for(var/obj/thing in pull_from) CHECK_TICK - if(thing.type in obj_blacklist) + if(obj_blacklist[thing.type]) continue var/metadata = generate_tgm_metadata(thing) current_header += "[empty ? "" : ",\n"][thing.type][metadata]" @@ -225,7 +250,7 @@ GLOBAL_LIST_INIT(save_file_chars, list( current_header += "[custom_data ? ",\n[custom_data]" : ""]" //====SAVING MOBS==== if(save_flag & SAVE_MOBS) - for(var/mob/living/thing in objects) + for(var/mob/living/thing in pull_from) CHECK_TICK if(istype(thing, /mob/living/carbon)) //Ignore people, but not animals continue @@ -234,65 +259,48 @@ GLOBAL_LIST_INIT(save_file_chars, list( empty = FALSE current_header += "[empty ? "" : ",\n"][place],\n[location])\n" //====Fill the contents file==== - //Compression is done here - var/position_of_header = header_dat.Find(current_header) - if(position_of_header) - //If the header has already been saved, change the character to the other saved header - header_char = header_chars[position_of_header] - else - header += "\"[header_char]\" = [current_header]" - header_chars += header_char - header_dat += current_header - index ++ - contents += "[header_char]\n" + var/textiftied_header = current_header.Join() + // If we already know this header just use its key, otherwise we gotta make a new one + var/key = header_data[textiftied_header] + if(!key) + key = calculate_tgm_header_index(key_index, layers) + key_index++ + header += "\"[key]\" = [textiftied_header]" + header_data[textiftied_header] = key + contents += "[key]\n" contents += "\"}" - return "//[DMM2TGM_MESSAGE]\n[header][contents]" + return "//[DMM2TGM_MESSAGE]\n[header.Join()][contents.Join()]" -//vars_to_save = list() to save all vars /proc/generate_tgm_metadata(atom/object) - var/dat = "" - var/data_to_add = list() + var/list/data_to_add = list() + var/list/vars_to_save = object.get_save_vars() - if(!vars_to_save) - return - for(var/variable in object.vars) + for(var/variable in vars_to_save) CHECK_TICK - if(!(variable in vars_to_save)) - continue var/value = object.vars[variable] - if(!value) - continue if(value == initial(object.vars[variable]) || !issaved(object.vars[variable])) continue if(variable == "icon_state" && object.smoothing_flags) continue - var/symbol = "" - if(istext(value)) - symbol = "\"" - value = hashtag_newlines_and_tabs(value, list("{"="", "}"="", "\""="", ";"="", ","="")) - else if(islist(value)) - value = to_list_string(value) - else if(isicon(value) || isfile(value)) - symbol = "'" - else if(!(isnum(value) || ispath(value))) - continue - //Prevent symbols from being because otherwise you can name something [";},/obj/item/gun/energy/laser/instakill{name="da epic gun] and spawn yourself an instakill gun. - data_to_add += "[variable] = [symbol][value][symbol]" - //Process data to add - var/first = TRUE - for(var/data in data_to_add) - dat += "[first ? "" : ";\n"]\t[data]" - first = FALSE - if(dat) - dat = "{\n[dat]\n\t}" - return dat -/proc/calculate_tgm_header_index(index, layers) - var/output = "" - for(var/i in 1 to layers) - CHECK_TICK - var/length = GLOB.save_file_chars.len + var/text_value = tgm_encode(value) + if(!text_value) + continue + data_to_add += "[variable] = [text_value]" + + if(!length(data_to_add)) + return + return "{\n\t[data_to_add.Join(";\n\t")]\n\t}" + +// Could be inlined, not a massive cost tho so it's fine +/// Generates a key matching our index +/proc/calculate_tgm_header_index(index, key_length) + var/list/output = list() + // We want to stick the first one last, so we walk backwards + var/list/pull_from = GLOB.save_file_chars + var/length = length(pull_from) + for(var/i in key_length to 1 step -1) var/calculated = FLOOR((index-1) / (length ** (i - 1)), 1) calculated = (calculated % length) + 1 - output = "[GLOB.save_file_chars[calculated]][output]" - return output + output += pull_from[calculated] + return output.Join() diff --git a/code/modules/buildmode/submodes/map_export.dm b/code/modules/buildmode/submodes/map_export.dm index 99412781680..e0cb6629e19 100644 --- a/code/modules/buildmode/submodes/map_export.dm +++ b/code/modules/buildmode/submodes/map_export.dm @@ -5,8 +5,6 @@ var/shuttle_flag = SAVE_SHUTTLEAREA_DONTCARE /// Variable with a flag value to indicate what should be saved (for example, only objects or only mobs). var/save_flag = ALL - /// A guard variable to prevent more than one map export process from occurring at the same time. - var/static/is_running = FALSE /datum/buildmode_mode/map_export/change_settings(client/builder) var/static/list/options = list( @@ -52,14 +50,25 @@ if(cornerA == cornerB) return - if(is_running) - to_chat(builder, span_warning("Someone is already running the generator! Try again in a little bit.")) + INVOKE_ASYNC(GLOBAL_PROC, GLOBAL_PROC_REF(_save_map), cornerA, cornerB, save_flag, shuttle_flag) + +/// A guard variable to prevent more than one map export process from occurring at the same time. +GLOBAL_VAR_INIT(map_writing_running, FALSE) +/// Hey bud don't call this directly, it exists so we can invoke async and prevent the buildmode datum being qdel'd from halting this proc +/proc/_save_map(turf/cornerA, turf/cornerB, save_flag, shuttle_flag) + if(!check_rights(R_DEBUG)) + message_admins("[ckey(usr)] tried to run the map save generator but was rejected due to insufficient perms.") + to_chat(usr, span_warning("You must have +ADMIN rights to use this.")) + return + if(GLOB.map_writing_running) + to_chat(usr, span_warning("Someone is already running the generator! Try again in a little bit.")) return - to_chat(builder, span_warning("Saving, please wait...")) - is_running = TRUE + to_chat(usr, span_warning("Saving, please wait...")) + GLOB.map_writing_running = TRUE - log_admin("Build Mode: [key_name(builder)] is exporting the map area from [AREACOORD(cornerA)] through [AREACOORD(cornerB)]") //I put this before the actual saving of the map because it likely won't log if it crashes the fucking server + //I put this before the actual saving of the map because it likely won't log if it crashes the fucking server + log_admin("Build Mode: [key_name(usr)] is exporting the map area from [AREACOORD(cornerA)] through [AREACOORD(cornerB)]") //oversimplified for readability and understandibility @@ -74,9 +83,9 @@ //Step 1: Get the data (This can take a while) var/dat = write_map(minx, miny, minz, maxx, maxy, maxz, save_flag, shuttle_flag) - //Step 2: Write the data to a file and give map to client + //Step 2: Write the data to a file and give map to client var/date = time2text(world.timeofday, "YYYY-MM-DD_hh-mm-ss") - var/file_name = sanitize_filename(tgui_input_text(builder, "Filename?", "Map Exporter", "exported_map_[date]")) - send_exported_map(builder, file_name, dat) - to_chat(builder, span_green("The map was successfully saved!")) - is_running = FALSE + var/file_name = sanitize_filename(tgui_input_text(usr, "Filename?", "Map Exporter", "exported_map_[date]")) + send_exported_map(usr, file_name, dat) + to_chat(usr, span_green("The map was successfully saved!")) + GLOB.map_writing_running = FALSE diff --git a/code/modules/mapping/reader.dm b/code/modules/mapping/reader.dm index ab6743ff17a..5ed29cabcdb 100644 --- a/code/modules/mapping/reader.dm +++ b/code/modules/mapping/reader.dm @@ -1004,8 +1004,6 @@ GLOBAL_LIST_EMPTY(map_model_default) if (!text) return - // If we're using a semi colon, we can do this as splittext rather then constant calls to find_next_delimiter_position - // This does make the code a bit harder to read, but saves a good bit of time so suck it up var/position var/old_position = 1 while(position != 0)