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.

Sign up for GitHub

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

Jump to bottom

Optimize startup time #97

Open
wants to merge 2 commits into
base: 1.21.1
Choose a base branch
from
+68 −35
Open

Optimize startup time #97

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dd052c
Optimize startup time
mezz Oct 8, 2024
3dbc624
Merge branch '1.21.1' into optimize-load
mezz Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions Common/src/main/java/com/almostreliable/unified/config/DuplicateConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,35 @@ public boolean shouldIgnoreRecipe(RecipeLink recipe) {
* @return True if the recipe type is ignored, false otherwise
*/
private boolean isRecipeTypeIgnored(RecipeLink recipe) {
return ignoredRecipeTypesCache.computeIfAbsent(recipe.getType(), type -> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gets called so much that the capturing lambda actually caused a minor performance hit, so I turned this computeIfAbsent into a regular get/put sequence

for (Pattern ignorePattern : ignoreRecipeTypes) {
if (ignorePattern.matcher(type.toString()).matches()) {
return true;
}
ResourceLocation type = recipe.getType();
Boolean ignored = ignoredRecipeTypesCache.get(type);
if (ignored == null) {
ignored = computeIsRecipeTypeIgnored(recipe);
ignoredRecipeTypesCache.put(type, ignored);
}
return ignored;
}

private boolean computeIsRecipeTypeIgnored(RecipeLink recipe) {
ResourceLocation type = recipe.getType();
String typeString = type.toString();
for (Pattern ignorePattern : ignoreRecipeTypes) {
if (ignorePattern.matcher(typeString).matches()) {
return true;
}
return false;
});
}
return false;
}

public JsonCompare.CompareSettings getCompareSettings(ResourceLocation type) {
return overrideRules.getOrDefault(type, defaultRules);
}

public JsonCompare.CompareContext getCompareContext(RecipeLink recipe) {
JsonCompare.CompareSettings compareSettings = getCompareSettings(recipe.getType());
return JsonCompare.CompareContext.create(compareSettings, recipe);
}

public boolean shouldCompareAll() {
return compareAll;
}
Expand Down
31 changes: 20 additions & 11 deletions Common/src/main/java/com/almostreliable/unified/unification/recipe/RecipeLink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,19 @@
import org.jetbrains.annotations.Nullable;

import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

public class RecipeLink implements RecipeData {
/**
* This cache is an optimization to avoid creating many ResourceLocations for just a few different types.
* Having fewer ResourceLocation instances can greatly speed up equality checking when these are used as map keys.
*/
private static final Map<String, ResourceLocation> PARSED_TYPE_CACHE = new HashMap<>();

private final ResourceLocation id;
private final ResourceLocation type;
private final JsonObject originalRecipe;
Expand All @@ -27,7 +35,8 @@ public RecipeLink(ResourceLocation id, JsonObject originalRecipe) {
this.originalRecipe = originalRecipe;

try {
this.type = ResourceLocation.tryParse(originalRecipe.get("type").getAsString());
String typeString = originalRecipe.get("type").getAsString();
this.type = PARSED_TYPE_CACHE.computeIfAbsent(typeString, ResourceLocation::parse);
} catch (Exception e) {
throw new IllegalArgumentException("could not detect recipe type");
}
Expand All @@ -40,19 +49,19 @@ public RecipeLink(ResourceLocation id, JsonObject originalRecipe) {
*
* @param first first recipe to compare
* @param second second recipe to compare
* @param compareSettings Settings to use for comparison.
* @param compareContext Settings and context to use for comparison.
* @return the recipe where rules are applied and the recipes are compared for equality, or null if the recipes are not equal
*/
@Nullable
public static RecipeLink compare(RecipeLink first, RecipeLink second, JsonCompare.CompareSettings compareSettings) {
public static RecipeLink compare(RecipeLink first, RecipeLink second, JsonCompare.CompareContext compareContext) {
JsonObject selfActual = first.getActual();
JsonObject toCompareActual = second.getActual();

JsonObject compare = null;
if (first.getType().toString().equals("minecraft:crafting_shaped")) {
compare = JsonCompare.compareShaped(selfActual, toCompareActual, compareSettings);
} else if (JsonCompare.matches(selfActual, toCompareActual, compareSettings)) {
compare = JsonCompare.compare(compareSettings.getRules(), selfActual, toCompareActual);
compare = JsonCompare.compareShaped(selfActual, toCompareActual, compareContext);
} else if (JsonCompare.matches(selfActual, toCompareActual, compareContext)) {
compare = JsonCompare.compare(compareContext.settings().getRules(), selfActual, toCompareActual);
}

if (compare == null) return null;
Expand Down Expand Up @@ -129,10 +138,10 @@ public String toString() {
* the master from the link will be used. Otherwise, we will create a new link if needed.
*
* @param otherRecipe Recipe data to check for duplicate against.
* @param compareSettings Settings to use for comparison.
* @param compareContext Settings and context to use for comparison.
* @return True if recipe is a duplicate, false otherwise.
*/
public boolean handleDuplicate(RecipeLink otherRecipe, JsonCompare.CompareSettings compareSettings) {
public boolean handleDuplicate(RecipeLink otherRecipe, JsonCompare.CompareContext compareContext) {
DuplicateLink selfDuplicate = getDuplicateLink();
DuplicateLink otherDuplicate = otherRecipe.getDuplicateLink();

Expand All @@ -141,7 +150,7 @@ public boolean handleDuplicate(RecipeLink otherRecipe, JsonCompare.CompareSettin
}

if (selfDuplicate == null && otherDuplicate == null) {
RecipeLink compare = compare(this, otherRecipe, compareSettings);
RecipeLink compare = compare(this, otherRecipe, compareContext);
if (compare == null) {
return false;
}
Expand All @@ -153,7 +162,7 @@ public boolean handleDuplicate(RecipeLink otherRecipe, JsonCompare.CompareSettin
}

if (otherDuplicate != null) {
RecipeLink compare = compare(this, otherDuplicate.getMaster(), compareSettings);
RecipeLink compare = compare(this, otherDuplicate.getMaster(), compareContext);
if (compare == null) {
return false;
}
Expand All @@ -163,7 +172,7 @@ public boolean handleDuplicate(RecipeLink otherRecipe, JsonCompare.CompareSettin
}

// selfDuplicate != null
RecipeLink compare = compare(selfDuplicate.getMaster(), otherRecipe, compareSettings);
RecipeLink compare = compare(selfDuplicate.getMaster(), otherRecipe, compareContext);
if (compare == null) {
return false;
}
Expand Down
5 changes: 3 additions & 2 deletions Common/src/main/java/com/almostreliable/unified/unification/recipe/RecipeTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ private boolean handleDuplicate(RecipeLink curRecipe, List<RecipeLink> recipes)
return false;
}

JsonCompare.CompareSettings compareSettings = duplicateConfig.getCompareSettings(curRecipe.getType());
JsonCompare.CompareContext compareContext = duplicateConfig.getCompareContext(curRecipe);

boolean foundDuplicate = false;
for (RecipeLink recipeLink : recipes) {
if (!curRecipe.getType().equals(recipeLink.getType())) {
Expand All @@ -150,7 +151,7 @@ private boolean handleDuplicate(RecipeLink curRecipe, List<RecipeLink> recipes)
continue;
}

foundDuplicate |= curRecipe.handleDuplicate(recipeLink, compareSettings);
foundDuplicate |= curRecipe.handleDuplicate(recipeLink, compareContext);
}

return foundDuplicate;
Expand Down
39 changes: 24 additions & 15 deletions Common/src/main/java/com/almostreliable/unified/utils/JsonCompare.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.almostreliable.unified.utils;

import com.almostreliable.unified.unification.recipe.RecipeLink;

import com.google.gson.JsonArray;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
Expand All @@ -8,8 +10,6 @@
import org.jetbrains.annotations.Nullable;

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -47,8 +47,8 @@ public static JsonObject compare(Map<String, Rule> rules, JsonObject... jsonObje
}

@Nullable
public static JsonObject compareShaped(JsonObject first, JsonObject second, CompareSettings compareSettings) {
if (!matches(first, second, compareSettings)) return null;
public static JsonObject compareShaped(JsonObject first, JsonObject second, CompareContext compareContext) {
if (!matches(first, second, compareContext)) return null;

JsonArray firstPattern = JsonUtils.arrayOrSelf(first.get("pattern"));
JsonArray secondPattern = JsonUtils.arrayOrSelf(second.get("pattern"));
Expand Down Expand Up @@ -97,20 +97,18 @@ private static Map<Character, JsonObject> createShapedKeyMap(JsonObject json) {
return keyMap;
}

public static boolean matches(JsonObject first, JsonObject second, CompareSettings compareSettings) {
Collection<String> ignoredFields = compareSettings.getIgnoredFields();
if (ignoredFields.isEmpty() && first.size() != second.size()) {
public static boolean matches(JsonObject first, JsonObject second, CompareContext compareContext) {
CompareSettings compareSettings = compareContext.settings;
if (!compareSettings.hasIgnoredFields() && first.size() != second.size()) {
return false;
}

for (Map.Entry<String, JsonElement> firstEntry : first.entrySet()) {
if (ignoredFields.contains(firstEntry.getKey())) continue;

JsonElement firstElem = firstEntry.getValue();
JsonElement secondElem = second.get(firstEntry.getKey());

for (String field : compareContext.compareFields()) {
JsonElement secondElem = second.get(field);
if (secondElem == null) return false;

JsonElement firstElem = first.get(field);

// sanitize elements for implicit counts of 1
if (compareSettings.handleImplicitCounts && needsSanitizing(firstElem, secondElem)) {
firstElem = sanitize(firstElem);
Expand Down Expand Up @@ -270,6 +268,17 @@ public String getName() {
}
}

public record CompareContext(CompareSettings settings, List<String> compareFields) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason you are using a list instead of a set here? Could even be a collection or iterable only. I am just asking because inside the create method, you retrieve the recipe key set and when the ignored fields are not empty, you copy the collection twice. Once inside the new HashSet and then with List.copyOf inside the CTor call. Is is because you want to make it immutable again?

Copy link
Author

@mezz mezz Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is focused on efficiency of the hot path, which in this case (according to profiling) is the path that iterates over the compareFields. We can sloppily make lots of copies elsewhere and it'll take less than 1ms in total, but if we iterate over a HashSet in a hot loop it will take longer than a List for example, so the main requirement is to enforce compareFields is a type that is fast to iterate over.

The Set is used earlier because it's a quick way to remove any duplicates and remove the ignored fields. You can do the same thing in many ways but there are tradeoffs between copying, contains performance, and clarity. Since the code there isn't called as much, I think I just tried to make it easy to read.

public static CompareContext create(CompareSettings settings, RecipeLink curRecipe) {
Set<String> compareFields = curRecipe.getActual().keySet();
if (!settings.ignoredFields.isEmpty()) {
compareFields = new HashSet<>(compareFields);
compareFields.removeAll(settings.ignoredFields);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the ignored fields happens so frequently that it slows down loading.
It is much faster to do it once here and keeping a list of the fields we need to compare.

}
return new CompareContext(settings, List.copyOf(compareFields));
}
}

public static class CompareSettings {

public static final String IGNORED_FIELDS = "ignored_fields";
Expand All @@ -292,8 +301,8 @@ public void addRule(String key, Rule rule) {
}
}

public Set<String> getIgnoredFields() {
return Collections.unmodifiableSet(ignoredFields);
public boolean hasIgnoredFields() {
return !ignoredFields.isEmpty();
}

public JsonObject serialize() {
Expand Down