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

Add Folia support #384

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

Conversation

pontaoski
Copy link
Contributor

Fixes #383.

@NeumimTo
Copy link

any update on this?

@stefvanschie
Copy link
Owner

Thank you for your pull request. Unfortunately, I believe this will not suffice to make IF work correctly with Folia, for the following reasons.

I think we should first consider how we want plugins that use IF to interact with it on Folia servers. Currently on Bukkit-based servers, all calls to IF code must be done from the main server thread. Any calls from a different thread give no guarantee that any method performs according to its contract. In return, any calls we make to your code (e.g., a click handler on GuiItem, a drag handler on Gui, etc.) will also be on the main thread.

I think there are several options that could work with regards to Folia support. The first one is to keep it like this, expecting all plugins to use a specific thread (GlobalRegionScheduler probably makes the most sense?) and in return all their code is also called on this thread. The advantage of this is that all the internal code for panes, guis, InventoryComponents and possibly more can stay as they are, working in a single-threaded manner. Only when we go from the player to IF (e.g., handling an inventory click event) or from IF to the player (e.g., copying the items from InventoryComponent over to the player's inventory) would require us to write code to switch over to a different thread. The disadvantage is that you need this context switch and plugins using IF that may not be in the specific thread currently will need to write this themselves.
Adding these context switches to IF itself is possible, but would be a lot of code for little gain - I don't really want this. We could consider adding assertions everywhere to ensure the plugin is calling the code from the right thread.
We can make some stuff support multiple threads in places that plugins aren't intended to touch anyway, such as HumanEntityCache, gaining a bit of a performance boost on Folia (and probably a bit of a performance decrease on Bukkit).

Another option is to rewrite everything so every single thing supports multiple threads. Replace every collection with an appropriate collection that supports multiple threads, replace fields with atomic variants, make sure all of the methods changes are done in a way where they seem atomic, etc. etc. etc. This is a lot of work, but would avoid the context switching needed when code from an arbitrary thread tries to interact with IF code. There may still be a context switch when IF code tries to interact with players as there's no guarantee that the code IF is currently running on is the region thread for the player, but compared to the first option this would only be a possibility, not a guarantee. Plugins that create their own panes would need to write their pane such that it supports multiple threads. Handlers can be called on any thread, so plugins that require a certain thread would still need to handle this, but if their code doesn't require a specific thread, no context switch is needed. In the end, we should get a performance boost from the reduced amount of context switching, probably with a decrease in performance on Bukkit-based servers, due to all the overhead from the code needed to make multiple threads play nicely with each other. Plugins could now also call methods from multiple threads on Bukkit, though, although the usefulness of this is probably pretty limited.

I think it makes most sense to go with option one, since it's significantly easier. Given this, there are many changes that need to be made.

Gui#update()
Inside the loop going over all the viewers, we need to schedule the code that changes the cursor item on that human entity's scheduler. We first need to change the cursor item on the human entity scheduler. We need to wait until that is finished, so we'd need to implement a busy wait until the ExecutionState of the ScheduledTask is FINISHED. Then we can call show on the global region thread for which we again need to wait until it's finished. Probably the easiest way to achieve this is to explicitly schedule this on the global region scheduler, so that we can again busy wait until the task is finished. Then we can follow this up by another context switch to the human entity scheduler to reset the cursor item again. These tasks should be collected in a local collection and afterwards we should wait until all of these tasks have finished.

AnvilGui#show(HumanEntity), BarrelGui#show(HumanEntity), BeaconGui#show(HumanEntity), BlastFurnaceGui#show(HumanEntity), BrewingStandGui#show(HumanEntity), CartographyTableGui#show(HumanEntity), ChestGui#show(HumanEntity), CraftingTableGui#show(HumanEntity), DispenserGui#show(HumanEntity), DropperGui#show(HumanEntity), EnchantingTableGui#show(HumanEntity), EnderChestGui#show(HumanEntity), FurnaceGui#show(HumanEntity), GrindstoneGui#show(HumanEntity), HopperGui#show(HumanEntity), MerchantGui#show(HumanEntity), ModernSmithingTableGui#show(HumanEntity), ShulkerBoxGui#show(HumanEntity), SmithingTableGui#show(HumanEntity), SmokerGui#show(HumanEntity), and StonecutterGui#show(HumanEntity)
The getPlayerInventoryComponent().placeItems call should be performed on the scheduler of the human entity. We should busy wait until it has finished at the end of the method.

BarrelGui#show(HumanEntity), BeaconGui#show(HumanEntity), BlastFurnaceGui#show(HumanEntity), BrewingStandGui#show(HumanEntity), CartographyTableGui#show(HumanEntity), ChestGui#show(HumanEntity), CraftingTableGui#show(HumanEntity), DispenserGui#show(HumanEntity), DropperGui#show(HumanEntity), EnchantingTableGui#show(HumanEntity), EnderChestGui#show(HumanEntity), FurnaceGui#show(HumanEntity), HopperGui#show(HumanEntity), ShulkerBoxGui#show(HumanEntity), SmithingTableGui#show(HumanEntity), SmokerGui#show(HumanEntity), and StonecutterGui#show(HumanEntity)
The call to HumanEntity#openInventory(Inventory) should be done on the thread of the human entity's scheduler. We should wait until this has finished.

MerchantGui#addTrade(MerchantRecipe, int), and MerchantGui#addTrade(MerchantRecipe)
A note to the documentation should be added that the passed MerchantRecipe instance may only ever be used on the global region thread after this call.

GuiItem(ItemStack, Consumer, Plugin), GuiItem(ItemStack, Plugin). GuiItem(ItemStack, Consumer), GuiItem(ItemStack), and GuiItem(ItemStack, Consumer, Logger, NamespacedKey)
A note to the documentation should be added that the passed ItemStack instance may only ever be used on the global region thread after this invocation.

GuiListener
We can't listen to the inventory-related events as normal, since Folia doesn't ensure these inventories work correctly. We need to introduce a packet listener which listens to these events and, if they're of our guis, are not passed to the server. We should take over and ensure correct scheduling. The event should be reconstructed for our own purposes and then we can call the methods ourselves. This should all be done on the global region thread.

GuiListener#onEntityPickupItem(EntityPickupItemEvent)
The code from the getGui call until (and including) the line of the int leftOver = ... should be scheduled on the main thread. The thread this event was fired on should wait until this code has finished. If the return statement was reached, the entire method should return from its execution. The remaining code should be done on the initial thread again.

GuiListener#onPluginDisable(PluginDisableEvent)
Everything after the first initial check should be done on the global region. The viewer.closeInventory() line should be scheduled on the thread of the human entity. These tasks should be collected in a collection and after the end of the outer gui loop, we should wait until all these tasks have finished.

InventoryComponent#display(PlayerInventory, int)
Javadocs should be updated to state that it should be called on the scheduler of the human entity owning the inventory.

InventoryComponent#placeItems(PlayerInventory, int)
Javadocs should be updated to state that it should be called on the scheduler of the human entity owning the inventory.

HumanEntityCache#storeAndClear(HumanEntity)
The code after the store call should be done on the human entity's scheduler and we should wait for it to finish.

HumanEntityCache#store(HumanEntity)
The loop should be done on the scheduler of the human entity and after that is done, the storing should be done on the global region thread again.

HumanEntityCache#restore(HumanEntity)
The loop should be done on the human entity's scheduler and we should wait for that to finish.

NMS
Essentially, code interacting with the player/human entity should be done on that scheduler. We should busy wait to ensure it's finished. Only 1.19.4 and later needs updating, though, since Folia didn't exist prior to that.

Please do note that we also want compatibility with Bukkit, so all this would need to be done such that it falls back to running normally if on Bukkit (similar to what you've done currently, but probably needs to be extended).

Copy link
Owner

@stefvanschie stefvanschie left a comment

Choose a reason for hiding this comment

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

See above comment

@pontaoski pontaoski force-pushed the work/jblackquill/folia-support branch from 669082a to d4eced8 Compare May 9, 2024 01:33
@pontaoski pontaoski force-pushed the work/jblackquill/folia-support branch from d4eced8 to 87bb8ac Compare August 24, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Folia
4 participants