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.

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

Skip recipe checks if input contains only programmed circuits. #3305

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

AbdielKavash
Copy link
Member

Background

Many machines use programmed circuits as a non-consumed input in their recipes. To automate such a process, the player typically leaves a circuit in the machine's input bus permanently, and inserts other inputs as needed. Moreover, certain "universal" setups rely on keeping several circuits in the input bus, and use recipe check order to pick the correct recipe when input items are inserted. As an example, one can run an electric blast furnace with circuits [1] and [11] in the bottom-right slots of an input bus; and when the EBF receives an input dust, it will use a circuit [11] recipe if available, otherwise a circuit [1] recipe, and finally a recipe without any circuits. This allows for automating many different recipes using only one machine.

The downside to this setup is that circuits in an input bus are still treated as items for the purpose of recipe checks (and this applies to phantom circuits set in the input bus's UI as well). This means that a machine with several circuits in its input bus iterates over its recipe map over and over with each recipe check, even though from the player's point of view the machine is effectively idle. When scaled up, and especially with machines with long recipe lists (EBF, LCR, ...), this starts having noticeable impact on tick times (see Evaluation section below). In fact I have noticed this in my survival world with Volcanuses and MCRs.

This PR modifies recipe checks in such a way that programmed circuits are no longer counted towards the number of items in the input bus. This means that if the input bus contains only circuits, it is treated as being empty, and thus recipe checks are completely skipped.

I note that this only specifically affects GT programmed circuits, and not the GT++ bio or breakthrough circuits, or other similar items (shapes, molds, lenses etc.) A more generic solution could be developed, but I wanted to keep the impact of this change minimal for now.

Evaluation

I have tested the performance impact in a new test world, with 256 Electric Blast Furnaces, with input buses configured as in the table below. In all cases except for the last circuits are the only items in the EBF's input, and therefore it is impossible for it to run any recipes. Comparison is made between 2.5.7-beta-2, and the same pack with only this change added. I note that the table only shows the average time. Since recipe checks are not run every tick, the actual impact on maximal tick time (which contributes to TPS spikes) is even larger than the table shows. Spark profiler results are included.

The number in the table is specifically time spent in MTEMultiBlockBase.checkProcessing().

Bus Configuration 2.5.0-beta-2 With this PR
Empty 0.02ms https://spark.lucko.me/MqHeYmdJzs 0.03ms https://spark.lucko.me/Lj1bNnKjxT
Phantom circuit 0.17ms https://spark.lucko.me/WofiQ5CHhh 0.02ms https://spark.lucko.me/drpBHnGPx0
One phantom and one real circuit 0.38ms https://spark.lucko.me/P2DHmDpd2i 0.02ms https://spark.lucko.me/y85W2mejnn
16 real circuits 0.76ms https://spark.lucko.me/Oge2FuBdR7 0.02ms https://spark.lucko.me/xxC9cwEHRV
16 non-circuit items 0.14ms https://spark.lucko.me/uJH4GNEJuv 0.14ms https://spark.lucko.me/ow2fc1ulUY

Caveats

Since every input item is now tested for being a circuit, this change could have a negligible performance effect in case there are many non-circuit items in the input bus. However, this is not the case in a typical setup; and in my testing I was unable to measure any difference; see last row of table above.

More importantly, this change modifies the semantics of RecipeMapBuilder.minInputs(). Previously the minimum number of item inputs included any programmed circuits which were a part of the recipe. After this change, since circuits are ignored when counting input items, the minimum number of inputs must be set to the number of "real" (non-circuit) items in any recipe.

For example, previously a Bending Machine required a minimum of 2 inputs: a metal ingot, and a programmed circuit. With this change, the minimum number of inputs is set to 1 (the ingot), since the circuit is no longer counted.

I did my best diligence to update the minInputs() calls everywhere this makes a difference, and I tested several machines in the beta version to ensure that all recipes still run correctly. However I would appreciate someone double-checking that I haven't missed anything.

Finally, if the only thing in a machine's inputs is a circuit (no other items and no fluids), recipe checks are skipped completely. This means that if a recipe takes no inputs other than a circuit, it will not be able to be processed correctly. This is currently the case for only one machine: the GT++ Matter Fabricator multiblock. The Rock Breaker and Zhuhai Fishing Port have similar recipes; however they implement their own processing logic and so they are not affected.

I have added a skip to this last check in form of RecipeMapBuilder.allowCircuitOnly() (see GTPPRecipeMaps.java:38) so that these recipes work correctly; this is used for the Matter Fabricator, but it can also be used for any future machines which might need such recipes.

@AbdielKavash AbdielKavash requested a review from a team October 1, 2024 10:57
@AbdielKavash AbdielKavash added the enhancement Improve an existing mechanic. Please explain the change with a before/after comparison. label Oct 1, 2024
@boubou19
Copy link
Member

i like the idea, but idk how you managed to pull out the data for your table. For instance, i get from this link https://spark.lucko.me/P2DHmDpd2i 988 ms over 120 000 ms (2 min of profile time) which is 0.76% of the CPU load.
Not that it really matters, as checking the spark profiles give the same trend.

This look quite promising, however, idk much about the backend processing much to approve or requests changes.

@AbdielKavash
Copy link
Member Author

i like the idea, but idk how you managed to pull out the data for your table. For instance, i get from this link https://spark.lucko.me/P2DHmDpd2i 988 ms over 120 000 ms (2 min of profile time) which is 0.76% of the CPU load. Not that it really matters, as checking the spark profiles give the same trend.

This is what I'm looking at:

@RecursivePineapple
Copy link
Contributor

How have you tested this? I think we'll have to scream test it on the nightlies regardless, but a quick sanity check on the common machines would be good.

@AbdielKavash
Copy link
Member Author

How have you tested this? I think we'll have to scream test it on the nightlies regardless, but a quick sanity check on the common machines would be good.

I have tested that the machine can still in fact run recipes, both with and without circuits in the input bus. I have done this for both multiblocks (EBF, bender, some other representatives of edge cases) and singleblock versions of the same. I have also explicitly tested the Matter Fabricator multi.

If there is anything else that you'd like me to check, let me know.

@wlhlm
Copy link
Member

wlhlm commented Nov 17, 2024

At this point, we should consider this for after 2.7.

@Alexdoru
Copy link
Member

At this point, we should consider this for after 2.7.

I second this opinion, it doesn't fix any bug but has high changes of introducing new ones. High risk low benefit stuff

@boubou19 boubou19 added the ongoing freeze - do not merge PR tagged with this do not meet the requirement to be merged during a freeze. label Nov 17, 2024
@AbdielKavash
Copy link
Member Author

Sounds good; I will add it to my private instance as soon as I update and watch for any issues.

@Dream-Master Dream-Master removed the ongoing freeze - do not merge PR tagged with this do not meet the requirement to be merged during a freeze. label Dec 8, 2024
@serenibyss
Copy link
Member

Sounds good; I will add it to my private instance as soon as I update and watch for any issues.

@AbdielKavash were you able to test it like this? How did it go?

@jdlovins
Copy link

Four helped me get this going as i'm in late UMV / early UXV on 2.7 and was hitting about 50mspt and spiking to over 100mspt due to prass spam. So far things are crafting and working as expected but did run into what i dont think is an issue, but more of an observation.

I have an array of forming presses to rename items for assline. If you have a crib with a mold, and the ghost circuit set it will not craft the renamed item. Once i removed the ghost circuit it crafted as expected. It's not a problem as the ghost circuit is unnecessary but maybe something to think about if it will cause issues elsewhere. I'll report back if I run into any major issues.

image
image

Copy link
Member

@serenibyss serenibyss left a comment

Choose a reason for hiding this comment

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

This is definitely an issue that needs to be fixed before this can merge, thanks for the feedback @jdlovins

@jdlovins
Copy link

Well, It's only been about a day but figured i would write some feedback before I forget. Throughout the last 24 ish hours of crafting things I didnt run into any issues except for the one above. Everything worked as expected and smoothly really.

However I ran into the zircaloy-4 issue, which four said was fixed in main alongside with a different TPS patch. So i decided to grab main and see how that compared to Kavashes recipe change and they seem comparable so far. I'm averaging the same ~30mspt between the two versions and cant tell much of a different at a glance (which is good!)

I know this is very napkin math comparison without sparks/profilers to back it up, but since I am likely going to stay on main I wanted to give some feedback from using it.

@Dream-Master Dream-Master requested a review from a team December 21, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve an existing mechanic. Please explain the change with a before/after comparison.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants