[MIRROR] Micro-optimize GetIdFromArguments to be 48% faster, gaining 0.48s of init time on local (likely more in prod) [MDB IGNORE] (#16039)

* Micro-optimize GetIdFromArguments to be 48% faster, gaining 0.48s of init time on local (likely more in prod) (#69659)

About The Pull Request

    Avoids stringifying key unless its necessary. This was done redundantly twice, but I locked it to just the isnum path, as REF will always return a string, and the other path passes istext.
    Use sortTim directly instead of sort_list. sort_list is just sortTim but it copies the list, so it's just wasted cost.

I still would like the bespoke element key option, as that's the only way to drastically cut down costs on things like item descriptions and decals, but this is good for the general use case, and makes it marginally less pressing.

I also want to test if we'd be better off inserting into the list in sorted order rather than sorting it all in the end, but I suspect not.

* Micro-optimize GetIdFromArguments to be 48% faster, gaining 0.48s of init time on local (likely more in prod)

Co-authored-by: Mothblocks <35135081+Mothblocks@users.noreply.github.com>

code/controllers/subsystem/dcs.dm

@@ -33,22 +33,28 @@ PROCESSING_SUBSYSTEM_DEF(dcs)
 	var/datum/element/eletype = arguments[1]
 	var/list/fullid = list("[eletype]")
 	var/list/named_arguments = list()
+
 	for(var/i in initial(eletype.id_arg_index) to length(arguments))
 		var/key = arguments[i]
-		var/value
+
 		if(istext(key))
-			value = arguments[key]
+			var/value = arguments[key]
-		if(!(istext(key) || isnum(key)))
+			if (isnull(value))
-			key = REF(key)
+				fullid += key
-		key = "[key]" // Key is stringified so numbers dont break things
+			else
-		if(!isnull(value))
+				if (!istext(value) && !isnum(value))
-			if(!(istext(value) || isnum(value)))
+					value = REF(value)
-				value = REF(value)
+				named_arguments[key] = value
-			named_arguments["[key]"] = value
+
-		else
+			continue
+
+		if (isnum(key))
 			fullid += "[key]"
+		else
+			fullid += REF(key)
 
 	if(length(named_arguments))
-		named_arguments = sort_list(named_arguments)
+		named_arguments = sortTim(named_arguments, /proc/cmp_text_asc)
 		fullid += named_arguments
+
 	return list2params(fullid)

code/modules/unit_tests/_unit_tests.dm

@@ -90,6 +90,7 @@
 #include "connect_loc.dm"
 #include "crayons.dm"
 #include "create_and_destroy.dm"
+#include "dcs_get_id_from_elements.dm"
 #include "designs.dm"
 #include "dummy_spawn.dm"
 #include "dynamic_ruleset_sanity.dm"

code/modules/unit_tests/dcs_get_id_from_elements.dm

@@ -0,0 +1,46 @@
+/// Tests that DCS' GetIdFromArguments works as expected with standard and odd cases
+/datum/unit_test/dcs_get_id_from_arguments
+
+/datum/unit_test/dcs_get_id_from_arguments/Run()
+	assert_equal(list(1), list(1))
+	assert_equal(list(1, 2), list(1, 2))
+	assert_equal(list(src), list(src))
+
+	assert_equal(
+		list(a = "x", b = "y", c = "z"),
+		list(b = "y", a = "x", c = "z"),
+		list(c = "z", a = "x", b = "y"),
+	)
+
+	TEST_ASSERT_NOTEQUAL(get_id_from_arguments(list(1, 2)), get_id_from_arguments(list(2, 1)), "Swapped arguments should not return the same id")
+	TEST_ASSERT_NOTEQUAL(get_id_from_arguments(list(1, a = "x")), get_id_from_arguments(list(1)), "Named arguments were ignored when creating ids")
+	TEST_ASSERT_NOTEQUAL(get_id_from_arguments(list(1, a = "x")), get_id_from_arguments(list(a = "x")), "Unnamed arguments were ignored when creating ids")
+	TEST_ASSERT_NOTEQUAL(get_id_from_arguments(list(src)), get_id_from_arguments(list(world)), "References to different datums should not return the same id")
+
+	TEST_ASSERT_NOTEQUAL(get_id_from_arguments(list()), SSdcs.GetIdFromArguments(list(/datum/element/dcs_get_id_from_arguments_mock_element2)), "Different elements should not match the same id")
+
+/datum/unit_test/dcs_get_id_from_arguments/proc/assert_equal(reference, ...)
+	var/result = get_id_from_arguments(reference)
+
+	// Start at 1 so the 2nd argument is 2
+	var/index = 1
+
+	for (var/other_case in args)
+		index += 1
+
+		var/other_result = get_id_from_arguments(other_case)
+
+		if (other_result == result)
+			continue
+
+		TEST_FAIL("Case #[index] produces a different GetIdFromArguments result from the first. [other_result] != [result]")
+
+/datum/unit_test/dcs_get_id_from_arguments/proc/get_id_from_arguments(list/arguments)
+	return SSdcs.GetIdFromArguments(list(/datum/element/dcs_get_id_from_arguments_mock_element) + arguments)
+
+// Necessary because GetIdFromArguments uses id_arg_index from an element type
+/datum/element/dcs_get_id_from_arguments_mock_element
+	id_arg_index = 2
+
+/datum/element/dcs_get_id_from_arguments_mock_element2
+	id_arg_index = 2