use composite buffers where possible #3737

Outfluencer · 2024-09-04T17:43:41Z

i think this should reduce memory allocations
I have tested it, its running without any issues for me.
But i did no perfromace tests for now, so they are welcome.

This should reduce memory allocations

Janmm14 · 2024-09-04T17:49:21Z

This should crash for native encryption for packets with size smaller than encryption threshold, as native encryption also requires a singular memory address which a composite bytebuf cannot deliver.

Outfluencer · 2024-09-04T17:52:21Z

oh you're probably right i tested it on an arm server (custom natives are not supported)

Janmm14 · 2024-09-05T11:55:36Z

The naming of fast boolean in my opinion should be something like canUseComposite.
And your complete removal from the length field prepender creates simplicity, but I think we could have it on fast and then if the CipherEncoder is added, we could set the prepender to's canUseComposite to false if we use natives, same for when adding compress.

BungeeCord/proxy/src/main/java/net/md_5/bungee/connection/InitialHandler.java

Line 510 in 84ac7ab

    
           ch.addBefore( PipelineUtils.FRAME_PREPENDER, PipelineUtils.ENCRYPT_HANDLER, new CipherEncoder( encrypt ) );

https://github.com/SpigotMC/BungeeCord/blob/master/proxy/src/main/java/net/md_5/bungee/netty/ChannelWrapper.java#L172

(i would just leave out the logic to re-enable canUseComposite on compress removal if cipher not present tho)

Outfluencer · 2024-09-05T12:03:41Z

The naming of fast boolean in my opinion should be something like canUseComposite. And your complete removal from the length field prepender creates simplicity, but I think we could have it on fast and then if the CipherEncoder is added, we could set the prepender to's canUseComposite to false if we use natives, same for when adding compress.

BungeeCord/proxy/src/main/java/net/md_5/bungee/connection/InitialHandler.java

Line 510 in 84ac7ab

ch.addBefore( PipelineUtils.FRAME_PREPENDER, PipelineUtils.ENCRYPT_HANDLER, new CipherEncoder( encrypt ) );

https://github.com/SpigotMC/BungeeCord/blob/master/proxy/src/main/java/net/md_5/bungee/netty/ChannelWrapper.java#L172
(i would just leave out the logic to re-enable canUseComposite on compress removal if cipher not present tho)

sounds logic and simple, i will change it.

Outfluencer · 2024-09-05T12:09:13Z

(i would just leave out the logic to re-enable canUseComposite on compress removal if cipher not present tho)

why isn't it simple to just set useComposite to true again?

Outfluencer · 2024-09-05T12:41:21Z

The naming of fast boolean in my opinion should be something like canUseComposite. And your complete removal from the length field prepender creates simplicity, but I think we could have it on fast and then if the CipherEncoder is added, we could set the prepender to's canUseComposite to false if we use natives, same for when adding compress.

The prepender is an instance (its shared), should we create a new one per connection, or should we add another class and switch them in the pipeline. I prefer per channel Varint21LengthFieldPrepender

Janmm14 · 2024-09-05T13:29:18Z

The naming of fast boolean in my opinion should be something like canUseComposite. And your complete removal from the length field prepender creates simplicity, but I think we could have it on fast and then if the CipherEncoder is added, we could set the prepender to's canUseComposite to false if we use natives, same for when adding compress.

The prepender is an instance (its shared), should we create a new one per connection, or should we add another class and switch them in the pipeline. I prefer per channel Varint21LengthFieldPrepender

Yeah I would un-share it (change to per-connection) instead of replacing the impl.

(i would just leave out the logic to re-enable canUseComposite on compress removal if cipher not present tho)

why isn't it simple to just set useComposite to true again?

I thought that, as the minecraft protocol does not support this and when it was supported it could cause problems due to missing acknowledgement packet (as the threshold applies both ways), we could just not care about this potential performance penalty when compression is removed.

Outfluencer · 2024-09-05T13:33:04Z

I don't think that it could break anything. As there are no raceconditions because evey operation is performed on the same thread right? (its not possible that somehow this boolean is changed while a packet write or read in the pipeline is processed)
It can only change the encoder behaviour of the next packet sent

But i will test what happens if i change the compression multiple times

Janmm14 · 2024-09-05T16:17:37Z

The naming of fast boolean in my opinion should be something like canUseComposite. And your complete removal from the length field prepender creates simplicity, but I think we could have it on fast and then if the CipherEncoder is added, we could set the prepender to's canUseComposite to false if we use natives, same for when adding compress.

The prepender is an instance (its shared), should we create a new one per connection, or should we add another class and switch them in the pipeline. I prefer per channel Varint21LengthFieldPrepender

Oh yeah then the prepender should be per-channel.

(i would just leave out the logic to re-enable canUseComposite on compress removal if cipher not present tho)

why isn't it simple to just set useComposite to true again?

Well its possible, doesn't care, but as its a code path that's essentially unused

proxy/src/main/java/net/md_5/bungee/netty/ChannelWrapper.java

Janmm14 · 2024-09-06T15:28:29Z

purely codewise looks good to me now!

md-5 · 2024-09-08T04:03:40Z

protocol/src/main/java/net/md_5/bungee/protocol/Varint21LengthFieldPrepender.java

+        {
+            ByteBuf buf = ctx.alloc().directBuffer( headerLen );
+            DefinedPacket.writeVarInt( bodyLen, buf );
+            list.add( ctx.alloc().compositeBuffer( 2 ).addComponents( true, buf, msg.retain() ) );


Is there any reason why this isn't a direct composite but the compressor is?

By default it should use direct in the impl but i can just call directCompositeBuffer method.
I will change it

andrewkm · 2024-09-12T02:45:18Z

@md-5 this commit is causing all sorts of issues with various plugins, as has been reported to me by a few other server owners, not to mention it broke floodgate as well, thus I can't even properly test myself.

md-5 · 2024-09-12T03:24:52Z

If it's a plugin not using the API then it's not really a bungee issue. Given that networking isn't exposed to plugins this is almost certainly the case.

Outfluencer · 2024-09-12T06:48:09Z

@md-5 this commit is causing all sorts of issues with various plugins, as has been reported to me by a few other server owners, not to mention it broke floodgate as well, thus I can't even properly test myself.

Floodgate accessed a private static final field that does not exist anymore.

Outfluencer · 2024-09-14T13:00:37Z

@andrewkm i fixed the floodgate issue

andrewkm · 2024-09-15T22:43:35Z

@andrewkm i fixed the floodgate issue

Thank you! :)

use composite buffers where possible

a3265f5

This should reduce memory allocations

don't use composite buffers if we need a memory address

16f0445

Outfluencer added 3 commits September 5, 2024 19:45

use compose if possible

0672bee

fix import and remove new line

ede0984

fix another import

05300c6

Janmm14 reviewed Sep 6, 2024

View reviewed changes

proxy/src/main/java/net/md_5/bungee/netty/ChannelWrapper.java Outdated Show resolved Hide resolved

Fix variable name

e62fe96

md-5 self-assigned this Sep 8, 2024

md-5 reviewed Sep 8, 2024

View reviewed changes

use compositeDirectBuffer

785e829

md-5 approved these changes Sep 8, 2024

View reviewed changes

md-5 mentioned this pull request Sep 8, 2024

Online mode server hangs on encrypting rather than sensible error message #3741

Closed

4 tasks

md-5 pushed a commit that referenced this pull request Sep 9, 2024

#3737: Use composite buffers where possible

b309e4a

md-5 closed this Sep 9, 2024

Tim203 mentioned this pull request Sep 9, 2024

Latest version of BungeeCord gives an error GeyserMC/Floodgate#544

Closed

Outfluencer mentioned this pull request Sep 14, 2024

update BungeeCord injector GeyserMC/Floodgate#546

Merged

Krakenied mentioned this pull request Sep 23, 2024

2.5.1 on a Bungee cord version is completely broken retrooper/packetevents#1007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use composite buffers where possible #3737

use composite buffers where possible #3737

Outfluencer commented Sep 4, 2024

Janmm14 commented Sep 4, 2024 •

edited

Loading

Outfluencer commented Sep 4, 2024

Janmm14 commented Sep 5, 2024

Outfluencer commented Sep 5, 2024 •

edited

Loading

Outfluencer commented Sep 5, 2024 •

edited

Loading

Outfluencer commented Sep 5, 2024

Janmm14 commented Sep 5, 2024

Outfluencer commented Sep 5, 2024 •

edited

Loading

Janmm14 commented Sep 5, 2024

Janmm14 commented Sep 6, 2024

md-5 Sep 8, 2024

Outfluencer Sep 8, 2024 •

edited

Loading

andrewkm commented Sep 12, 2024 •

edited

Loading

md-5 commented Sep 12, 2024

Outfluencer commented Sep 12, 2024

Outfluencer commented Sep 14, 2024

andrewkm commented Sep 15, 2024

use composite buffers where possible #3737

use composite buffers where possible #3737

Conversation

Outfluencer commented Sep 4, 2024

Janmm14 commented Sep 4, 2024 • edited Loading

Outfluencer commented Sep 4, 2024

Janmm14 commented Sep 5, 2024

Outfluencer commented Sep 5, 2024 • edited Loading

Outfluencer commented Sep 5, 2024 • edited Loading

Outfluencer commented Sep 5, 2024

Janmm14 commented Sep 5, 2024

Outfluencer commented Sep 5, 2024 • edited Loading

Janmm14 commented Sep 5, 2024

Janmm14 commented Sep 6, 2024

md-5 Sep 8, 2024

Choose a reason for hiding this comment

Outfluencer Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

andrewkm commented Sep 12, 2024 • edited Loading

md-5 commented Sep 12, 2024

Outfluencer commented Sep 12, 2024

Outfluencer commented Sep 14, 2024

andrewkm commented Sep 15, 2024

Janmm14 commented Sep 4, 2024 •

edited

Loading

Outfluencer commented Sep 5, 2024 •

edited

Loading

Outfluencer commented Sep 5, 2024 •

edited

Loading

Outfluencer commented Sep 5, 2024 •

edited

Loading

Outfluencer Sep 8, 2024 •

edited

Loading

andrewkm commented Sep 12, 2024 •

edited

Loading