WIP - Ticker Rewrite

[TheBusyBiscuit]

Description

This Pull Request was originally created by @LinoxGH, see #2290.

We have decided that it is not ready to merged into master yet as there is still a lot that needs to be tweaked. That's why I am reopening it under a new branch inside the main repository.

Proposed changes

This makes some changes to the way TickerTask schedules its tasks.

Checklist

• [ ] I have fully tested the proposed changes and promise that they will not break everything into chaos.
• [ ] I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
• [x] I followed the existing code standards and didn't mess up the formatting.
• [x] I did my best to add documentation to any public classes or methods I added.
• [x] I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
• [ ] I added sufficient Unit Tests to cover my code.

[github-actions[bot]]

Your Pull Request was automatically labelled as: "💡 Performance Optimization" Thank you for contributing to this project! ❤️

[WalshyDev]

ConcurrentHashMap is very good with high levels of concurrency, I'm wondering if these locks will hurt more than help. We don't have many threads accessing this no?

@md5sha256 what are your thoughts on this?

[md5sha256]

Which locks are you referring to? The ones which lock the queues for copy + block storage stuff?

[WalshyDev]

We lock writes and reads a lot, ConcurrentHashMap works in segments so it can handle multiple threads reading and writing but if they're in the same segment it gets synchronized. From my initial thinking our locks on this map is doing more damage than good.

We lock every read and write which means only 1 thread max at a time.

I'm not sure how often the stuff here is modified by other threads but it's definitely something to consider

[md5sha256]

Ah yes so basically we lock some of the queues due to the underlying Location objects being mutable. I have a WIP thing which adds more detailed docs among others to the method detail.

It might be worth just converting everything to BlockPosition... HOWEVER, there is a cost to performance giving we have to switch to-and-fro for BlockStorage.

[WalshyDev]

We need to get rid of all Locations in this code haha

[md5sha256]

I can do another PR which just converts everything to block pos but from the last time I had a go at it my concern was that we'd just be converting from BlockPosition to Location literally every interaction. Whilst that saves us from having to use locks we basically just have to use cpu cycles on converting stuff. Not to mention, we'd be querying the WeakReference which BlockPosition uses

[WalshyDev]

I'd need to take a lot at what happens, we shouldn't need to convert from Location to BlockPosition a lot but I guess it depends where we use it.

BlockStorage rewrite will help this all out a lot

[md5sha256]

Yes. The rewrite will help for anything that needs immutable locations. Its just a bit of a waste to have to keep converting to keep compatibility with API sometimes.

[md5sha256]

I'd like to request for master to be merged in once again

[WalshyDev]

Done

[md5sha256]

Thanks!

[md5sha256]

I think this can now be un-marked as a draft.

[TheBusyBiscuit]

I think this can now be un-marked as a draft.

Compilation failure
Error:  /home/runner/work/Slimefun4/Slimefun4/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/tasks/TickerTask.java:[438,39] unreported exception java.lang.InterruptedException; must be caught or declared to be thrown

[md5sha256]

What... this compiled fine for me yesterday

[md5sha256]

Ok now that this compiles... can it be un-marked as a draft?

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!

1 Bug
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

31.0% Coverage
0.0% Duplication

[TheBusyBiscuit]

Things to address:

• This (https://sonarcloud.io/project/issues?id=Slimefun_Slimefun4&open=AXqAp2_rnVsrledxAx54&pullRequest=2996&resolved=false&types=BUG)
• Unit Tests

[md5sha256]

About thread interruption: I suppose we actually do need to re-interrupt the thread for TickerTask#halt. I misread something regarding re-throwing the exception or re-interrupting, sonarcloud is correct.

[md5sha256]

Is unit testing the only thing missing on this PR or are there other issues to be resolved?

[TheBusyBiscuit]

Is unit testing the only thing missing on this PR or are there other issues to be resolved?

I wish I knew

[J3fftw1]

closing this as stale. Keeping the branch alive however