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

Threading conundrum #2

Closed
Chocohead opened this issue Apr 5, 2020 · 3 comments
Closed

Threading conundrum #2

Chocohead opened this issue Apr 5, 2020 · 3 comments

Comments

@Chocohead
Copy link

I'm in the situation where I have potentially a significant number of runtime assets which are relatively slow to read which all need giving to RRP. With the way this is designed I would've thought the rrp_pre entrypoint would work, yet its implementation is just a serial loop in another thread. Whilst there is nothing wrong with this, it leaves me with a choice of three scenarios:

  • Use the rrp_pre entrypoint and block the thread until the initial reading (which is still on thread from a different preLaunch entrypoint) is done, then add the resources.
    • This is obviously non-ideal as now other resource generators are being held up by me waiting on the main thread. They probably won't be held up for that long, but I don't know how long they might take either.
  • Use the preLaunch entrypoint and add things asynchronously in my own thread
    • This is non-ideal as it won't hold up resource loading if it happens to be slow (as only RRPPre.PRE_GEN_LOCK can do that). Potentially this is not important as the resources may not be accessed until later, but I cannot guarantee this for them all nor have a way of knowing for a given resource if this is the case.
    • As an aside the non-thread safe methods in RuntimeResourcePack should probably warn of it given they are not backed by thread safe collections. It's all quite innocent in RRPPreGenEntrypoint which suggests they are all safe enough to call which could make for some strange CMEs from the main thread.
  • Use the preLaunch entrypoint and add things synchronously
    • This is non-ideal as it will hold the whole game up whilst I add things. Avoids any problems with being too slow for resource loading I guess ;)

I have a feeling everything isn't designed with threading in mind (which is totally fine. it can be a pain to design for), but if speed is the goal to use this over Artifice it shouldn't be left taped on IMO. I have given it a little thought about possible solutions at least if you'd like some ideas:

  • Move to using a thread pool to handle asynchronous additions (possibly RuntimeResourcePack#EXECUTOR_SERVICE)
    • This allows shutting down the pool when it is time for the resources to be done, preventing late submissions.
    • This fixes the spin loop in ReloadableResourceManagerImplMixin#registerRDP which is a bit naughty.
  • Stop using EntrypointUtils and instead parallelise the List returned by FabricLoader#getEntrypoints.
    • This avoids people shouting at you for using internal APIs and fixes the entrypoints blocking each other if some are slower than others
  • Wrap everything up in Futures and allow mods to chain their resources into the loading process
    • The Mojang way it feels like, not that you should necessarily listen to them :P
    • Probably just a messier way of having a thread pool by abstracting it all
    • Potentially easier for a modder to use if the external abstraction is clear (not that you couldn't just abstract a thread pool well directly)
  • Go completely wild and allow other mods to register their intentions to a Phaser and then use that as a form of self asserted thread pooling.
    • Not entirely necessary if there's already a thread pool, but allows greater flexibility for other mods to use their own (optionally threaded) implementations.
    • Also means the threads don't have to start until the work for them is ready, or even at all.
    • Does open the possibility for the game to hang if a mod fails to announce their completion re-arriving or deregistering, but you can force Phasers to stun terminate if they're being slow/you want them to with a single method call.

There's probably an argument for an entirely threaded form of RuntimeResourcePack, maybe even using Semaphores to limit concurrent writes or just as a lazy way of de-threading calls (or a ReentrantReadWriteLock which would work for that task too), but maybe there's a point of diminishing returns eventually where it doesn't quite need to be that complicated. All food for thought in making the generation faster at least.

@Devan-Kerman
Copy link
Owner

using the same executor service won't work in the event a mod decides to wait on an async task for whatever reason. I should have used a lock, and for some reason didn't. I think rrp is in need a big upgrade though, since modders can't choose the priority of the resource pack, along with the inefficiencies of using String#format for templating, and then creating a byte array from that

@Devan-Kerman
Copy link
Owner

I've rewritten the whole thing from scratch, will commit soon:tm: once wiki is done, I think the threading is much better this time

@Devan-Kerman
Copy link
Owner

Well there you go, all done I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants