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

Feature: Better AFK #87

Closed
wants to merge 3 commits into from
Closed

Feature: Better AFK #87

wants to merge 3 commits into from

Conversation

rainbowdashlabs
Copy link
Contributor

I saw #84 and decided to provide a proposal for the implementation.

The current implementation will spread the afk check into 20 ticks. At the end of 20 ticks every player will be checked one time.
The implementation also provides lagg and will jump to the next tick if for some reasons the current calculation takes too long. However this shouldnt happen.

The current implementation compares the hased location of the last check and the current location and decides on the base of the hash if the player has moved in any way or not.

I also changed the cooldown and playerActivity to Instants. These provide a more solid and more readable solution to measure durations between time points.

@nkomarn
Copy link
Owner

nkomarn commented Mar 21, 2021

Thanks for the contribution! This implementation is really clean, definitely a lot nicer than how I would've likely gone about it. There is one consideration of note, though: would it be possible to make the checker task run asynchronously so we don't have to worry about eating too much of the tick? Realistically, even 20ms of tick time for checking player location changes is far too much (20 out of 50ms for a stable 20 tps).

We would have to make sure the queue is a thread-safe or concurrent implementation if we do go that route. Also, maybe ticking the task less often wouldn't be a bad idea since the data isn't awfully critical.

@rainbowdashlabs
Copy link
Contributor Author

Working with the bukkit api asynchronously is normally always a bad idea.
Of course reading from it is way safer than actually writing or changing values from an async thread. However I think that this would be too much effort to make.
I tend to implement safety points quite often. Even when I dont think that they will ever come in action. Probably the 20 ms cap allows the conclusion that the operation could really take this long. So lets take the math.

The current max player amount which can be safely run on one server is around 100-150 players.
We can hash a lot of locations in 1 ms. I hashed around 100 locations on a single thread in this time. (tested this with a small benchmark). The hash is the most expensive operation in the afk check.
If we know assume that the server can take the double amount of players we still would have to divide the checked locations by 20.
In this case we would check 15 locations per tick. This is nothing and beyond any concern regarding performance.

Running a task every tick is really if they are not calculation heave, which this task obviously isn't.

Running it async can even worsen the performance. Starting a new thread is a really expensive operation. Creating a new task for such a small operation isnt really a profitable idea. especially since we only check 15 locations in the worst case.

tldr; Async always sounds fancy, but is normally not required and can even produce overhead for thread creation.
The current calculations are way to small to deserve an own thread.

@rainbowdashlabs
Copy link
Contributor Author

Closing this.
Main part is merged in #88.Will probably commit some fixes in another PR since some of my code was made less smart.

@rainbowdashlabs rainbowdashlabs deleted the feature/better-afk branch July 10, 2021 01:24
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

Successfully merging this pull request may close these issues.

None yet

2 participants