Don't sort children on node addition to save on performance

[aikar]

Everytime a child is added to the CommandNode, the children was sorted.
This action is extremely heavy with large node trees such as Minecraft.

From what I can see, sort order was not even needed by type since
the dispatcher parse checks argument and literal nodes instead.

Testing on /bossbar command seemed to have no impact to behavior.

Credit to PaperMC/Paper#3172

Co-authored-by: virustotalop virustotalop@gmail.com

[aikar]

This issue causes significant CPU hits every time a player logs in or changes world, so it provided a substantial improvement.

See numbers here: PaperMC/Paper#3154 (comment)

[liach]

Hmm, it is good to improve load-time performance, but by switching to a tree map, how is the lookup performance at runtime? Minecraft doesn't add to the map any more at runtime, so the actual impact isn't that big.

Theoretically a tree map lookup shouldn't be slower than that of a linked hash map. Just need some profiling to show the actual performance at runtime (after all the command children are added).

[aikar]

I don't believe that map is even queried at runtime, the other maps are used instead.

But regardless, worrying about get performance is a premature optimization, as this is not hot code. Tree map will absolutely be fast enough for something that runs so infrequently even if it was queried.

[liach]

Conclusion of the debate above:
With aikar's current setup, he should just keep this patch in his pr
https://github.com/Mojang/brigadier/pull/68/files#diff-2eb7953fa0b13555eda5df1df0ac9e82L91-L92

The rest of replacing linked hash map with tree map is pointless.

The new tree map does not preserve the iteration order that the linked hash map could guarantee as the tree map is comparing the string key instead of the command node value as done in linked hash map previously.

[virustotalop]

The lines that are sorting the map was the original cause of the performance degradation. The idea was to use a TreeMap so that CommandNodes are sorted automatically. When I made the original patch it didn't seem like the behavior was changed due to LiteralCommandNode sorting by the name of the node anyways. I'm not super familiar with the structure of brigadier or how command nodes are resolved and how this section would effect behavior using a TreeMap instead of checking for a LiteralCommandNode first. I have ran this patch in production as well as using a TreeSet and a HashMap as discussed here and both seem to act the same at least to the client. This should probably be investigated further by someone who is more familiar with brigadier as I'm not sure how this would work for other servers, mods etc.

[liach]

Oh, that section would be prioritizing literal command nodes over other types of command nodes (presumably only argument command nodes), and then sort each type of node by their sorted key.

brigadier/src/main/java/com/mojang/brigadier/tree/CommandNode.java

Lines 173 to 180 in 9542f80

	@Override
	public int compareTo(final CommandNode<S> o) {
	if (this instanceof LiteralCommandNode == o instanceof LiteralCommandNode) {
	return getSortedKey().compareTo(o.getSortedKey());
	}
	return (o instanceof LiteralCommandNode) ? 1 : -1;
	}

For instance, if we have subchildren apple <banana> cactus (<> denotes argument nodes), the original sort order would be apple cactus <banana> (literals first), while yours would be apple <banana> cactus (literal mixed with argument, sorted only by name)

[liach]

For those looking for priority of nodes, they should rather call getRelevantNodes and check than depending on the iteration order here.

[liach]

Hmm, something actually happens here! Interesting 👀