Potential ModifiedCraftingRecipe recipe matching performance problem

[quat1024]

Noticed this while looking at a reddit poster's spark. They have an unusual performance problem (some wacky datapack running /clear every tick) which happens to put strain on the recipe matching system.

My casual understanding is that ModifiedCraftingRecipe works like this:

• ModifiedCraftingRecipe#matches calls getOriginalRecipe
  

  apoli/src/main/java/io/github/apace100/apoli/util/ModifiedCraftingRecipe.java

  Line 42 in a417c4a

  RecipeEntry<CraftingRecipe> originalRecipe = getOriginalRecipe(craftingInventory)

• getOriginalRecipe loops through every recipe in the game and calls matches on it. In the worst case where none match, this will call matches on every recipe in the game
  

  apoli/src/main/java/io/github/apace100/apoli/util/ModifiedCraftingRecipe.java

  Lines 139 to 143 in a417c4a

  	return playerEntity.getServer().getRecipeManager().listAllOfType(RecipeType.CRAFTING)
  	.stream()
  	.filter(entry -> !(entry.value() instanceof ModifiedCraftingRecipe)
  	&& entry.value().matches(craftingInventory, playerEntity.getWorld()))
  	.findFirst();

• if one matches, then ModifiedCraftingPower#doesApply is called
  

  apoli/src/main/java/io/github/apace100/apoli/util/ModifiedCraftingRecipe.java

  Lines 51 to 53 in a417c4a

  	return getModifyCraftingPowers(craftingInventory)
  	.stream()
  	.anyMatch(mcp -> mcp.doesApply(id, resultStack));

• But MCP#doesApply checks the recipe ID anyway, so what'd ya loop over all recipes for
  

  apoli/src/main/java/io/github/apace100/apoli/power/ModifyCraftingPower.java

  Lines 55 to 56 in a417c4a

  	public boolean doesApply(Identifier recipeId, ItemStack originalResultStack) {
  	return (recipeIdentifier == null || recipeIdentifier.equals(recipeId))

There are probably far fewer ModifiedCraftingPowers than there are recipes, right, so I think it makes way more sense for these loops to go in the opposite order. First find MCPs relevant to the current player, then ask them which recipes they'd like to modify, and query omly those recipes in matches and craft.

Otherwise you're in the dubious situation of a ModifiedCraftingRecipe singlehandedly doubling worst-case recipe match time just by existing. This is probably what made it show up so clearly in that player's spark.

To add insult to injury, looks like RecipeManagerMixin ensures all the ModifiedCraftingRecipes are checked first:

apoli/src/main/java/io/github/apace100/apoli/mixin/RecipeManagerMixin.java

Lines 28 to 37 in a417c4a

	@ModifyReturnValue(method = "getFirstMatch(Lnet/minecraft/recipe/RecipeType;Lnet/minecraft/inventory/Inventory;Lnet/minecraft/world/World;)Ljava/util/Optional;", at = @At("RETURN"))
	private <C extends Inventory, T extends Recipe<C>> Optional<RecipeEntry<T>> apoli$prioritizeModifiedRecipes(Optional<RecipeEntry<T>> original, RecipeType<T> type, C inventory, World world) {
	return this.getAllOfType(type)
	.values()
	.stream()
	.filter(entry -> entry.value() instanceof ModifiedCraftingRecipe
	&& entry.value().matches(inventory, world))
	.findFirst()
	.or(() -> original);
	}

Also yeah it's like 4:30am, I could be missing something obvious or maybe there is a good reason for ModifiedCraftingRecipe to work like this

Edit

Yes I literally didn't see that a null Identiifier in ModifiedCraftingPower would modify all crafting recipes so you may actually need to match everything. Go to bed before filing issues, ya dummy.

Still, this feature is paid for even when it's not used, and I'm not sure people need to modify every crafting recipe

[eggohito]

The logic for modifying recipes have already been overhauled in the 1.21.x branch (see: #247), which eliminates the 2nd lookup and instead wraps the recipe (if it's a crafting recipe, that is) in a wrapper (ModifiedCraftingRecipe is now a wrapper, with an Identifier for the ID of the recipe entry, and a CraftingRecipe which is where the most of the logic is delegated to)

Although I'm not sure how much this would improve performance (haven't really got the time to benchmark), this change should at least lessen the load 🤔 but feel free to test it and/or share any suggestions to make the implementation better 👍