Improve CompostBinRecipe#getRecipe loading time

[democat3457]

Replaces a for loop with a registry getValue call.

If a modpack has lots of food items, there can be a lot of values in the CompostBinRecipe registry. Since the CompostBinRecipe.getRecipe call is called for every food item, the method alone can take a while to execute - to add to that, the getRecipe method is called in the TooltipEvent, which is called for every item. This can produce an extra loading time of about 40 seconds as it iterates over every food value for every item in the game.

By replacing the costly for loop with a simpler getValue call, the method's execution saves a lot of processing.

[CLAassistant]

All committers have signed the CLA.

[codetaylor]

This looks good. I'll review it again tomorrow and go from there.

This looks like it would be a dramatic performance improvement when a lot of food items are involved. Do you have any profiling measurements taken after the modification?

You could probably improve it even further by removing the containsKey(...) calls and just leverage a null check on the result of the get(...) call since containsKey(...) and get(...) both invoke a redundant seekByKey(...) in the underlying HashBiMap.

  @Override
  public boolean containsKey(@Nullable Object key) {
    return seekByKey(key, smearedHash(key)) != null;
  }

  @Nullable
  @Override
  public V get(@Nullable Object key) {
    return Maps.valueOrNull(seekByKey(key, smearedHash(key)));
  }

The improvement would likely be negligible though.

[democat3457]

I don't have any profiling measurements after the modification because Pyrotech dev environment is a pain to setup :P (requiring Athenaeum, Dropt, and other lib/ jars just to build Pyrotech)

But I'll see if I can get it going nonetheless.

[democat3457]

You could probably improve it even further by removing the containsKey(...) calls and just leverage a null check on the result of the get(...) call since containsKey(...) and get(...) both invoke a redundant seekByKey(...) in the underlying HashBiMap.

Done.

[democat3457]

This looks like it would be a dramatic performance improvement when a lot of food items are involved. Do you have any profiling measurements taken after the modification?

Truly a dramatic difference.

[codetaylor]

Yeah, that's much better performance! I appreciate you taking the time to test your changes.

Logically, the changes appear sound, but I do have a few issues with the PR:

1. Commit messages should use the imperative mood in the subject line. Commit message subjects should be able to finish the sentence "If applied, these changes will..." Improve, Add, Remove, Change, Modify, Refactor, etc. Look at the project's commit history for more examples.

2. In commit https://github.com/codetaylor/pyrotech-1.12/pull/379/commits/739567017922f04c333f19ffef667e689a3d8799, use of the variable name ret does not follow the project's variable name conventions. Java is a verbose language, so let's lean into that. Variable names should not be arbitrary abbreviations of words. They should be verbose and meaningful such as result or recipe. In this case I would use recipe as it correlates with the recipe variable found in similar logic in the overloaded method with the same name.

3. In commit https://github.com/codetaylor/pyrotech-1.12/pull/379/commits/739567017922f04c333f19ffef667e689a3d8799, the comparison argument of the ternary operator should be encapsulated with parenthesis to follow the project's style conventions.

4. In commit https://github.com/codetaylor/pyrotech-1.12/pull/379/commits/739567017922f04c333f19ffef667e689a3d8799, four blank lines following method declarations should not have been removed. A single blank line after a method declaration conforms to the project's style conventions.

5. In commit https://github.com/codetaylor/pyrotech-1.12/pull/379/commits/87f3548b7815e3a0d04e360ab1012ac25f7b2a7d, changes were made to the build files that don't belong in this PR. Furthermore, the changes have been made with no explanation as to why they are necessary. I am interested as to why these changes were made as they might be important, but I am not interested in applying these changes with this PR. If these changes are important, I will go ahead and make them myself, but I must first understand why.

   • The change to .gitignore is pretty self explanatory. If VSCode creates only a single folder, .vscode in the root of the project, then the entry should be prepended with a slash to anchor the pattern match to the root: /.vscode/. If VSCode creates .vscode folders throughout the project's tree and they all need to be ignored, then omitting the leading slash will match all folders named .vscode in any subfolder.

   • Why was http://files.minecraftforge.net/maven changed to https://maven.minecraftforge.net and why is it important that this change be made?

   • What mystery changes were made to gradlew and why is it important?

So, this is how I would probably handle it.

First, create a new local branch patch-2 from patch-1 to preserve this history. Next, add a new commit that fixes 2, 3, and 4. Then fix 1 and 5 by performing an interactive rebase onto the master branch to squash all commits into one, remove the commit that adds build changes, and rename the final, single commit using the imperative mood - something like the title of this PR would be perfect. Finally, submit a new PR for patch-2.

If you're not comfortable rebasing, an alternative solution would be to just create a new branch patch-2 and manually copy over the changes from the previous work keeping in mind the points discussed above. Aim for a single commit with the imperative mood, don't make any unnecessary styling changes, and omit the changes to the build files.

[democat3457]

Fair points, will address these issues.

[democat3457]

To explain number 5 (commit 87f3548):

• .gitignore: it should indeed be /.vscode/ because vscode creates a folder only in the root.
• build.gradle: the link http://files.minecraftforge.net/maven is now deprecated and it's recommended to use https://maven.minecraftforge.net to point to the Forge maven.
• .gradlew: the file was not executable before, but the change was made to make it executable (chmod +x ./gradlew)

[democat3457]

Done, with a force push (hope that's okay)

[codetaylor]

Looks good, thanks for tightening that up. A force push seems fine. I couldn't remember if GitHub would retain the links to the original commits after a force push so I wanted to avoid it. Seems like it's all good though.

Thanks for explaining the changes you made to the build files also.

I'll merge, build, and release later tomorrow or early Monday.