Skip to content
This repository has been archived by the owner on May 29, 2022. It is now read-only.

New ticker and NetworkManager was updated #381

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

Conversation

Dysaido
Copy link
Contributor

@Dysaido Dysaido commented Mar 21, 2022

Description

Upgrade tick counter from MinecraftServer, upgrade queue from NetworkManager and Scheduler from bukkit. It is ReentrantBlockableEventLoop from 1.16.5 paper and it works better then 1.8.8 counter. I updated timings too. It works as good as before, just the protocolsupport can't running this version.

How has this been tested?

I tested it on my local with 2 account and I used fawe, I think the protocolsupport doesn't work this modification because MinecraftServer doesn't include Queue with futuretask. It fixes if you use viaversion.

Additional Comments

Just the better performance!

Checklist:

  • [ Y] I have reviewed my code thoroughly.
  • [ Y] I have tested my code.
  • [ X] My changes generate no new warnings

@wuangg
Copy link
Contributor

wuangg commented Mar 22, 2022

Have you done any benchmark before saying this PR could give a better performance?

@Dysaido
Copy link
Contributor Author

Dysaido commented Mar 22, 2022

Have you done any benchmark before saying this PR could give a better performance?

I have got an server with 30 players. It was running 3 month. Most process faster with this modification then default calculator, because we don't send the thread to sleep. The yield is a better solution then sleep. It is no coincidence that Mojang switched to this. NetworkManager and ServerConnectin supplemented with isPending and I removed ReentrantReadWriteLock from NetworkManager because it doesn't neccessary. I updated scheduler from new Paper. I removed database from Plugin, because nothing uses this. The benefits of this are clear.

@ghost
Copy link

ghost commented Mar 22, 2022

Can't you do another PR to do only the ticker thing? instead of combining every change (Replace to lambda, channel listener, adventureapi and etc)

@Dysaido
Copy link
Contributor Author

Dysaido commented Mar 22, 2022

Can't you do another PR to do only the ticker thing? instead of combining every change (Replace to lambda, channel listener, adventureapi and etc)

I need an guidance. Because I don't have enough experience about git. 😁

@sadcenter
Copy link
Contributor

sadcenter commented Mar 22, 2022

Simply, divide this PR into parts. So, don't create PR with things unrelated to each other, create multiple PRs
For example:
Instead of creating: New ticker and NetworkManager
Create: New Ticker, Updated NetworkManager and so on
You can do it by using different branches.

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

Successfully merging this pull request may close these issues.

3 participants