This is required mainly for async entity tracking and async support for protocollib.
Some performance metrics would be helpful but may be difficult to obtain
#(fullwall) Asyncify NPC#getTrait / metadata
bright bramble
keen axleBOT
(fullwall) Asyncify NPC#getTrait / metadata
keen axleBOT
This is required mainly for async entity tracking and async support for protocol...
Hi I'm AutoThreadBot! Don't mind me, I'll just be adding the helper team to this thread so they can see it. A human will get to you soon.
bright bramble
<@&315191201831714816> would be interested in your experience / thoughts. especially as this may result in making getTrait async-only
shadow crest
async only, as in, not allowed on main thread? that would break compatibility with everything ever that uses citizens
like - every citizens connected plugin uses getTrait, and all exclusively from main thread
bright bramble
async _only_, as in, not allowed on main thread? that would break compatibility ...
as in, internally use concurrenthashmap
which is a pretty big API change as well
the alternative being to add specific async-enabled APIs
shadow crest
as in, internally use concurrenthashmap
that in itself would be fine. Performance-wise, uh... I tested that many years ago for Denizen and it was slightly worse but i don't remember specifics
i don't see any reason you'd need separate async APIs from the existing sync ones, it would route to the same logic and both variants would need to be multithreading-safe anyway
Reliability wise, just, don't do what some of the spigotty minecraft internals did ie have lookups based on comparing Thread.getCurrentThread() to random stored copies and take arcane different routes depending
just using things like Concurrent(X) for sets and appropriately relevant volatile/atomic read/writes where relevant or just screw it synchronized where not easily done, would suffice. Will hurt perf and there's no way to avoid that without threading safety issues
the biggest thing is you gotta track every route that you're calling async and make sure every thing it can possibly touch is fully async safe on its own
bright bramble
i don't see any reason you'd need separate async APIs from the existing sync one...
I suppose the implicit assumption has been that trait code is sync only
shadow crest
for example, this called async:
MyTrait x = npc.getTrait();
// ... calc stuff
x.getNpc().etc()
that's a 1 in a million error that's gonna break some random poor soul's server when an NPC is removed exactly while it's calculating ie the npc is no longer attached.
You can assume nothing and must check everything
bright bramble
I know how async works :p
there's a few places in the codebase where the async accesses are a little... risky
anyway, it's more about the API contract
creating a specific API that's explicitly async-aware vs changing getTrait to allow async
and also the performance impact but it will be very hard to measure on a large server