pmmp / PocketMine-MP

A server software for Minecraft: Bedrock Edition in PHP
https://pmmp.io
GNU Lesser General Public License v3.0
3.28k stars 1.56k forks source link

Event tags #5705

Open dktapps opened 1 year ago

dktapps commented 1 year ago

Description

During a recent discussion on #internals-dev, event tags were discussed. This is a possible way to improve the performance of events such as DataPacketReceiveEvent.

Taking DataPacketReceiveEvent as an example, the event would be tagged with the name of the packet. An event handler could then declare a tag like @filter InventoryTransactionPacket in order to receive only events tagged with InventoryTransactionPacket.

This would enable avoiding work jumping through hoops in the RegisteredListener call stack just to have the event handler immediately return because it didn't have the desired packet.

Justification

DataPacketReceiveEvent and DataPacketSendEvent are very frequently fired (sometimes thousands of times per second, if the server is busy). Any way we can improve the performance of these events will benefit servers at scale.

Alternative methods

We could also just remove these events entirely...

dktapps commented 1 year ago

Related Discord discussion: https://discord.com/channels/373199722573201408/546255162222706696/1092754273680502794

aderoian commented 1 year ago

What if we scratch this event for the option to register a custom packet handler? Kind of like the current internal handlers like InGamePacketHandler, this would allow a plugin to override functions to listen for packets.

abstract class DefaultCustomPacketHander {
    public function handleInventoryTransactionPacket(NetworkSession $origin, InventoryTransactionPacket $packet) : void {

    }
}

This is an example class (which could have methods for every applicable packet) that would be extended to execute a block of code when that packet is sent.

Additionally, these methods could return a bool value to replace the option of "canceling the event". It could also be taken a step further to allow for multiple handlers and include an event when a handler is registered.

JavierLeon9966 commented 1 year ago

That would be similar to https://github.com/Muqsit/SimplePacketHandler, which can also be implemented.

dktapps commented 1 year ago

This issue isn't specifically about DataPacketReceiveEvent. We could perhaps implement it in a different way for receiving packets specifically, yes, but this high frequency problem affects various other events too, such as PlayerMoveEvent.

ShockedPlot7560 commented 1 year ago

What I would personally like to see:

Definition of filterable properties

To allow any event to benefit from this system, I think it is necessary to accept some flexibility on the declaration of what is accepted as a filter.
A tag type @filterProperty would allow to specify the name of the property we want to filter. It would take the name of the property.

A new interface FilterableEvent should be considered to allow better filtering when registering if we should parse the comments or not

Taking for example the DataPacketReceiveEvent. So we would have in class header:

/**
* @filterProperty packet
*/
class DataPacketReceiveEvent extends ServerEvent implements Cancellable, FilterableEvent

Definition of filters for listener

A bit like the event tags, we will specify the property and the value with a tag type: filter.

At first this would check just at the instance level but in the future we may see another tag allowing to specify the filter method to apply: integer, string, etc ...

So, if we want to register a new Listener to receive only the LoginPacket, we will have to apply this:

class MyListener implement Listener {
        /**
         * @priority LOW
         * @filter packet LoginPacket
         */
    public function onLoginPacket(DataPacketReceiveEvent $event) : void {
        return;
    }
}

Disclaimer

I do not have the best knowledge of the current event system, this is simply a proposal after noting the current system and the possibility of implementation.
It is also a first draft that may not be useful if the way to set up had already been defined before.

On the other hand, I think it will be necessary to modify the internal listener registration structure to distinguish two types of list:

This will avoid going too far in the stack and thus have the desired effect