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.

Sign up for GitHub

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

Jump to bottom

Optimize stack #828

Merged
merged 7 commits into from
Jul 27, 2024
Merged

Optimize stack #828

merged 7 commits into from
Jul 27, 2024

Conversation

AlexProgrammerDE
Copy link
Contributor

No description provided.

@Kas-tle
Copy link
Member

Kas-tle commented Jun 17, 2024

Can you explain why these changes are needed?

@AlexProgrammerDE
Copy link
Contributor Author

As explained on Discord, the main benefits are reduced allocations, using newer and faster Java 11+ NIO APIs and improved error-handling.

@AlexProgrammerDE
Copy link
Contributor Author

I'm working on a little extra stuff for this PR. We can heavily simplify the entire pipeline by switching to AttributeKeys for controlling encryption/compression from netty. That way we don't need to modify the pipeline after initialization.

@AlexProgrammerDE
Copy link
Contributor Author

AlexProgrammerDE commented Jun 18, 2024 •
edited
Loading

Nah, lets get merged first what is already here. I don't want the PR to get too big.But I roughly know how this should be implemented. Maybe I'll make it into another PR.

AlexProgrammerDE added a commit to AlexProgrammerDE/MCProtocolLib that referenced this pull request Jun 18, 2024
@AlexProgrammerDE
Improve pipeline
a0d65bb 
This simplifies all pipeline code and ensures some listeners like the sizer are always present. The code already assumed that the sizer is always there and thus causes issues. The sizer can be deactivated still now and has pretty much no performance losses from this. The profit from this PR is that there is less logic with modifying the PR and thus developers interacting with the channel can assume specific things about the order and placements of elements in the pipeline. This will be useful once ViaVersion is supported, and it is expected that certain elements always are in the pipeline and don't change. My plan is to also always have an encryption and compression handler in the pipeline that is controlled via AttributeKeys from netty, but for that first GeyserMC#828 needs to be merged. So this PR only completes the goal partially, but that's fine. PR is ready for review like it is right now.
@AlexProgrammerDE AlexProgrammerDE mentioned this pull request Jun 18, 2024
Merged
@Tim203
Copy link
Member

Tim203 commented Jul 1, 2024

Do you have some benchmarks of the before and after?

@AlexProgrammerDE
Copy link
Contributor Author

AlexProgrammerDE commented Jul 2, 2024 •
edited
Loading

Do you have some benchmarks of the before and after?

Unfortunately I don't have the tooling right now to test this properly.
But I can tell you about why this is faster.
For compression/decompression now nio buffers are used. That reduces the array allocations and works more closely with the deflater/inflater classes. That way operations are slightly faster.
For encryption the same methods are used, but it's faster because of removal of unnecessary array size allocations.
With the encryption algorithm used in mc the in and out size of the algorithm is the same and thus no extra arrays need to be allocated, we can simply just reuse the input buffer.

onebeastchris
onebeastchris approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@onebeastchris onebeastchris merged commit bb38c8a into GeyserMC:master Jul 27, 2024
1 check passed
onebeastchris added a commit that referenced this pull request Sep 10, 2024
@AlexProgrammerDE @Konicai @onebeastchris
Static sizer and timeout handlers in the pipeline (#833)
4148fa9 
* Improve pipeline

This simplifies all pipeline code and ensures some listeners like the sizer are always present. The code already assumed that the sizer is always there and thus causes issues. The sizer can be deactivated still now and has pretty much no performance losses from this. The profit from this PR is that there is less logic with modifying the PR and thus developers interacting with the channel can assume specific things about the order and placements of elements in the pipeline. This will be useful once ViaVersion is supported, and it is expected that certain elements always are in the pipeline and don't change. My plan is to also always have an encryption and compression handler in the pipeline that is controlled via AttributeKeys from netty, but for that first #828 needs to be merged. So this PR only completes the goal partially, but that's fine. PR is ready for review like it is right now.

* Revert some stuff

* Fix channel race condition

* Fix closing race condition

* Prevent client race conditions.

* Fix test failure, idk how, idk why, but it works now

* Address review

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/BuiltinFlags.java

Co-authored-by: Konicai <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/BuiltinFlags.java

Co-authored-by: Konicai <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/BuiltinFlags.java

Co-authored-by: chris <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpServer.java

Co-authored-by: chris <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/BuiltinFlags.java

Co-authored-by: chris <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/BuiltinFlags.java

Co-authored-by: chris <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpServer.java

Co-authored-by: chris <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpServer.java

Co-authored-by: chris <[email protected]>

* Update protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpServer.java

Co-authored-by: chris <[email protected]>

---------

Co-authored-by: Konicai <[email protected]>
Co-authored-by: chris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onebeastchris onebeastchris onebeastchris approved these changes

@Konicai Konicai Konicai approved these changes

@Redned235 Redned235 Redned235 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AlexProgrammerDE @Kas-tle @Tim203 @Redned235 @Konicai @onebeastchris