Avoid serializing object-based payload into `PacketByteBuf` on the main thread #3407

deirn · 2023-11-06T18:35:47Z

Resolves #3335

graph TD
  ResolvablePayload["<i>ResolvablePayload</i>"] --> RetainedPayload["RetainedPayload(Identifier, PacketByteBuf)"]
  
  ResolvablePayload --> ResolvedPayload["<i>ResolvedPayload</i>"]
  RetainedPayload -.-> ResolvedPayload
  
  ResolvedPayload --> TypedPayload["TypedPayload(FabricPacket)"]
  ResolvedPayload --> UntypedPayload["UntypedPayload(Identifier, PacketByteBuf)"]

On local connection (singleplayer), ResolvedPayload instances will be used directly, bypassing the serialization altogether.

On remote connection, the sender sends a ResolvedPayload instance. The receiver then receives a PacketByteBuf that gets retained and wrapped into RetainedPayload. This doesn't create a new buffer, unlike the current implementation.

Then the receiver checks for instance of ResolvablePayload and resolves it into ResolvedPayload if it's not yet as such.

For TypedPayload, PacketType#read will be called.
For UntypedPayload, the buffer from RetainedPayload will be copied into a new one.

Then the handler for the payload will be called as usual.

Technici4n · 2023-11-06T18:52:25Z

Does this add a way to enable serialization for singleplayer, for example in dev?

deirn · 2023-11-07T02:42:50Z

Does this add a way to enable serialization for singleplayer, for example in dev?

No, is it really needed? If you want to check for serialization you can always run the server.

deirn · 2023-11-07T02:46:46Z

One thing that I forgot to mention: if you send a packet using raw buffer when it's registered using the PacketType it will fail on local connection as it expects it to be TypedPayload when it is a UntypedPayload. Should we also handle the conversion?

Not true anymore, it will now convert between the two accordingly. Still, should it really do so?

modmuss50 · 2023-11-19T14:34:38Z

Does this add a way to enable serialization for singleplayer, for example in dev?

No, is it really needed? If you want to check for serialization you can always run the server.

I think by default it should always serialise when in dev, with a system property to control the behaviour.

...g-api-v1/src/main/java/net/fabricmc/fabric/mixin/networking/CustomPayloadC2SPacketMixin.java

modmuss50

PR is looking good now, I just want to test it out myself next. One small thing regarding the logging around the new system prop.

modmuss50 · 2023-11-20T09:27:27Z

fabric-networking-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/NetworkingImpl.java

+		if (FORCE_PACKET_SERIALIZATION) {
+			LOGGER.warn("Force Packet Serialization is enabled");
+			if (FabricLoader.getInstance().isDevelopmentEnvironment()) LOGGER.warn("This is the default on dev env");
+			LOGGER.warn("Disable it with -Dfabric-api.networking.force-packet-serialization=false");
+		}


Im not sure these need to be warning levels, or logged at all in development tbh.

How does one know to disable the serialization then? I guess most devs won't need to be concerned about this but still.

I would imagine that 99% of people wont care about this. A single .info log line would be fine, people see warnings and think they have done something wrong. Whats here makes it sound like it should be disabled not that it can be disabled.

Why would one want to disable it in the first place? ... ;)

To make sure it doesn't bug on singleplayer, because of improper/not using copy of an object?

modmuss50 · 2023-11-22T15:30:51Z

I will initally release this as a beta, many thanks for sorting this. 👍

…in thread (#3407) * channeled network addon refactor * checkstyle * fix junit tests * convert TypedPayload <-> UntypedPayload if necessary * assert payload size * add vm arg to force serialization * change log level to info and make it single line (cherry picked from commit 6225d43)

channeled network addon refactor

bf76eba

checkstyle

a112668

deirn added 2 commits November 7, 2023 09:55

fix junit tests

56eecf7

convert TypedPayload <-> UntypedPayload if necessary

7c3648e

deirn added the fabric-networking Pull requests and issues related to the networking api label Nov 7, 2023

deirn requested a review from a team November 7, 2023 15:42

modmuss50 self-requested a review November 19, 2023 14:31

modmuss50 reviewed Nov 19, 2023

View reviewed changes

...g-api-v1/src/main/java/net/fabricmc/fabric/mixin/networking/CustomPayloadC2SPacketMixin.java Outdated Show resolved Hide resolved

assert payload size

3573e5a

deirn force-pushed the channeled-addon-refactor branch from 8cf5c72 to 3573e5a Compare November 20, 2023 06:57

add vm arg to force serialization

48b5852

modmuss50 approved these changes Nov 20, 2023

View reviewed changes

change log level to info and make it single line

49505ea

modmuss50 approved these changes Nov 22, 2023

View reviewed changes

modmuss50 added the merge me please Pull requests that are ready to merge label Nov 22, 2023

modmuss50 merged commit 6225d43 into FabricMC:1.20.2 Nov 26, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid serializing object-based payload into `PacketByteBuf` on the main thread #3407

Avoid serializing object-based payload into `PacketByteBuf` on the main thread #3407

deirn commented Nov 6, 2023

Technici4n commented Nov 6, 2023

deirn commented Nov 7, 2023

deirn commented Nov 7, 2023 •

edited

Loading

modmuss50 commented Nov 19, 2023

modmuss50 left a comment

modmuss50 Nov 20, 2023

deirn Nov 20, 2023

modmuss50 Nov 21, 2023

Technici4n Nov 22, 2023

Patbox Nov 22, 2023 •

edited

Loading

modmuss50 commented Nov 22, 2023

Avoid serializing object-based payload into PacketByteBuf on the main thread #3407

Avoid serializing object-based payload into PacketByteBuf on the main thread #3407

Conversation

deirn commented Nov 6, 2023

Technici4n commented Nov 6, 2023

deirn commented Nov 7, 2023

deirn commented Nov 7, 2023 • edited Loading

modmuss50 commented Nov 19, 2023

modmuss50 left a comment

Choose a reason for hiding this comment

modmuss50 Nov 20, 2023

Choose a reason for hiding this comment

deirn Nov 20, 2023

Choose a reason for hiding this comment

modmuss50 Nov 21, 2023

Choose a reason for hiding this comment

Technici4n Nov 22, 2023

Choose a reason for hiding this comment

Patbox Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

modmuss50 commented Nov 22, 2023

Avoid serializing object-based payload into `PacketByteBuf` on the main thread #3407

Avoid serializing object-based payload into `PacketByteBuf` on the main thread #3407

deirn commented Nov 7, 2023 •

edited

Loading

Patbox Nov 22, 2023 •

edited

Loading