From e8c77af92b82c57a107b02b8900f4205c0a9485a Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sun, 23 Feb 2014 22:13:46 +0100 Subject: [PATCH] Get rid of the other weak maps, and use thread local fields instead. This should improve performance, and prevent the possibility of not informing plugins of sent packets. --- .../protocol/events/PacketEvent.java | 1 - .../injector/netty/ChannelInjector.java | 164 +++++++++--------- .../injector/netty/ChannelListener.java | 9 +- .../injector/netty/ClosedInjector.java | 15 -- .../protocol/injector/netty/Injector.java | 22 --- .../injector/netty/NettyProtocolInjector.java | 33 +--- 6 files changed, 93 insertions(+), 151 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java b/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java index bfd7d468..f4fbbb76 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java @@ -23,7 +23,6 @@ import java.io.ObjectOutputStream; import java.lang.ref.WeakReference; import java.util.EventObject; -import org.bukkit.Bukkit; import org.bukkit.entity.Player; import org.bukkit.event.Cancellable; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java index 64e2b935..3cae6906 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java @@ -52,6 +52,11 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { public static final ReportType REPORT_CANNOT_INTERCEPT_SERVER_PACKET = new ReportType("Unable to intercept a written server packet."); public static final ReportType REPORT_CANNOT_INTERCEPT_CLIENT_PACKET = new ReportType("Unable to intercept a read client packet."); + /** + * Indicates that a packet has bypassed packet listeners. + */ + private static final PacketEvent BYPASSED_PACKET = new PacketEvent(ChannelInjector.class); + // The login packet private static Class PACKET_LOGIN_CLIENT = null; private static FieldAccessor LOGIN_GAME_PROFILE = null; @@ -81,25 +86,22 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { // Known network markers private ConcurrentMap packetMarker = new MapMaker().weakKeys().makeMap(); - private ConcurrentMap markerEvent = new MapMaker().weakKeys().makeMap(); - - // Packets we have processed before - //private Set processedPackets = Collections.newSetFromMap(new MapMaker().weakKeys().makeMap()); - - // Packets to ignore - //private Set ignoredPackets = Collections.newSetFromMap(new MapMaker().weakKeys().makeMap()); /** - * Indicate that this packet will be ignored. + * Indicate that this packet has been processed by event listeners. *

* This must never be set outside the channel pipeline's thread. */ - private boolean processPackets = true; + private PacketEvent currentEvent; /** * A flag set by the main thread to indiciate that a packet should not be processed. */ - private boolean scheduleProcessPackets = true; + private final ThreadLocal scheduleProcessPackets = new ThreadLocal() { + protected Boolean initialValue() { + return true; + }; + }; // Other handlers private ByteToMessageDecoder vanillaDecoder; @@ -158,7 +160,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { vanillaEncoder = (MessageToByteEncoder) originalChannel.pipeline().get("encoder"); if (vanillaDecoder == null) - throw new IllegalArgumentException("Unable to find vanilla decoder.in " + originalChannel.pipeline() ); + throw new IllegalArgumentException("Unable to find vanilla decoder in " + originalChannel.pipeline() ); if (vanillaEncoder == null) throw new IllegalArgumentException("Unable to find vanilla encoder in " + originalChannel.pipeline() ); patchEncoder(vanillaEncoder); @@ -186,49 +188,61 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { channelField.setValue(new ChannelProxy(originalChannel, MinecraftReflection.getPacketClass()) { @Override protected Callable onMessageScheduled(final Callable callable, FieldAccessor packetAccessor) { - if (handleScheduled(callable, packetAccessor)) { - return new Callable() { - @Override - public T call() throws Exception { - T result = null; - - // This field must only be updated in the pipeline thread - processPackets = false; - result = callable.call(); - processPackets = true; - return result; - } - }; - } - return null; + final PacketEvent event = handleScheduled(callable, packetAccessor); + + // Handle cancelled events + if (event != null && event.isCancelled()) + return null; + + return new Callable() { + @Override + public T call() throws Exception { + T result = null; + + // This field must only be updated in the pipeline thread + currentEvent = event; + result = callable.call(); + currentEvent = null; + return result; + } + }; } @Override protected Runnable onMessageScheduled(final Runnable runnable, FieldAccessor packetAccessor) { - if (handleScheduled(runnable, packetAccessor)) { - return new Runnable() { - @Override - public void run() { - processPackets = false; - runnable.run(); - processPackets = true; - } - }; - } - return null; + final PacketEvent event = handleScheduled(runnable, packetAccessor); + + // Handle cancelled events + if (event != null && event.isCancelled()) + return null; + + return new Runnable() { + @Override + public void run() { + currentEvent = event; + runnable.run(); + currentEvent = null; + } + }; } - protected boolean handleScheduled(Object instance, FieldAccessor accessor) { + protected PacketEvent handleScheduled(Object instance, FieldAccessor accessor) { + // See if we've been instructed not to process packets + if (!scheduleProcessPackets.get()) + return BYPASSED_PACKET; + + // Let the filters handle this packet Object original = accessor.get(instance); - Object result = scheduleProcessPackets ? processSending(original) : original; - - if (result != null) { + PacketEvent event = processSending(original); + + if (event != null && !event.isCancelled()) { + Object changed = event.getPacket().getHandle(); + // Change packet to be scheduled - if (original != result) - accessor.set(instance, result); - return true; - } - return false; + if (original != changed) + accessor.set(instance, changed); + }; + return event != null ? event : BYPASSED_PACKET; } }); @@ -242,7 +256,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { * @param message - the message/packet. * @return The resulting message/packet. */ - private Object processSending(Object message) { + private PacketEvent processSending(Object message) { return processSending(message, packetMarker.get(message)); } @@ -251,7 +265,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { * @param message - the message/packet. * @return The resulting message/packet. */ - private Object processSending(Object message, NetworkMarker marker) { + private PacketEvent processSending(Object message, NetworkMarker marker) { return channelListener.onPacketSending(ChannelInjector.this, message, marker); } @@ -282,11 +296,11 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { */ protected void encode(ChannelHandlerContext ctx, Object packet, ByteBuf output) throws Exception { try { - NetworkMarker marker = getMarker(packet);; - PacketEvent event = markerEvent.remove(marker); + PacketEvent event = currentEvent; + NetworkMarker marker = null; // This packet has not been seen by the main thread - if (processPackets) { + if (event == null) { Class clazz = packet.getClass(); // Schedule the transmission on the main thread instead @@ -296,11 +310,13 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { packet = null; } else { - packet = processSending(packet, marker); - marker = getMarker(packet); - event = markerEvent.remove(marker); + event = processSending(packet, marker); } } + if (event != null) { + // Retrieve marker without accidentally constructing it + marker = NetworkMarker.getNetworkMarker(event); + } // Process output handler if (packet != null && event != null && NetworkMarker.hasOutputHandlers(marker)) { @@ -354,13 +370,15 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { byteBuffer.resetReaderIndex(); marker = new NettyNetworkMarker(ConnectionSide.CLIENT_SIDE, getBytes(byteBuffer)); } - Object output = channelListener.onPacketReceiving(this, input, marker); + PacketEvent output = channelListener.onPacketReceiving(this, input, marker); // Handle packet changes - if (output == null) - packets.clear(); - else if (output != input) - packets.set(0, output); + if (output != null) { + if (output.isCancelled()) + packets.clear(); + else if (output.getPacket().getHandle() != input) + packets.set(0, output.getPacket().getHandle()); + } } } catch (Exception e) { channelListener.getReporter().reportDetailed(this, @@ -441,9 +459,9 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { public void sendServerPacket(Object packet, NetworkMarker marker, boolean filtered) { saveMarker(packet, marker); - scheduleProcessPackets = filtered; + scheduleProcessPackets.set(filtered); invokeSendPacket(packet); - scheduleProcessPackets = true; + scheduleProcessPackets.set(true); } /** @@ -469,10 +487,11 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { Runnable action = new Runnable() { @Override public void run() { - Object result = filtered ? channelListener.onPacketReceiving(ChannelInjector.this, packet, marker) : packet; + PacketEvent event = filtered ? channelListener.onPacketReceiving(ChannelInjector.this, packet, marker) : null; + Object result = event != null ? event.getPacket().getHandle() : packet; // See if the packet has been cancelled - if (result == null) + if (event != null && event.isCancelled()) return; try { @@ -512,18 +531,6 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { return playerConnection; } - @Override - public boolean unignorePacket(Object packet) { - // NOP - return false; - } - - @Override - public boolean ignorePacket(Object packet) { - // NOP - return false; - } - @Override public NetworkMarker getMarker(Object packet) { return packetMarker.get(packet); @@ -535,13 +542,6 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { packetMarker.put(packet, marker); } } - - @Override - public void saveEvent(NetworkMarker marker, PacketEvent event) { - if (marker != null) { - markerEvent.put(marker, event); - } - } @Override public Player getPlayer() { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelListener.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelListener.java index 14160053..b3251fc8 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelListener.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelListener.java @@ -2,6 +2,7 @@ package com.comphenix.protocol.injector.netty; import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.events.NetworkMarker; +import com.comphenix.protocol.events.PacketEvent; /** @@ -16,9 +17,9 @@ interface ChannelListener { * @param injector - the channel injector. * @param packet - the packet. * @param marker - the associated network marker, if any. - * @return The new packet, if it should be changed, or NULL to cancel. + * @return The packet even that was passed to the listeners, with a possible packet change, or NULL. */ - public Object onPacketSending(Injector injector, Object packet, NetworkMarker marker); + public PacketEvent onPacketSending(Injector injector, Object packet, NetworkMarker marker); /** * Invoked when a packet is being received from a client. @@ -27,9 +28,9 @@ interface ChannelListener { * @param injector - the channel injector. * @param packet - the packet. * @param marker - the associated network marker, if any. - * @return The new packet, if it should be changed, or NULL to cancel. + * @return The packet even that was passed to the listeners, with a possible packet change, or NULL. */ - public Object onPacketReceiving(Injector injector, Object packet, NetworkMarker marker); + public PacketEvent onPacketReceiving(Injector injector, Object packet, NetworkMarker marker); /** * Determine if there is a packet listener for the given packet. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ClosedInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ClosedInjector.java index d6918d79..250002b2 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ClosedInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ClosedInjector.java @@ -47,16 +47,6 @@ class ClosedInjector implements Injector { return Protocol.HANDSHAKING; } - @Override - public boolean unignorePacket(Object packet) { - return false; - } - - @Override - public boolean ignorePacket(Object packet) { - return false; - } - @Override public NetworkMarker getMarker(Object packet) { return null; @@ -67,11 +57,6 @@ class ClosedInjector implements Injector { // Do nothing } - @Override - public void saveEvent(NetworkMarker marker, PacketEvent event) { - // Do nothing - } - @Override public void setUpdatedPlayer(Player player) { // Do nothing diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/Injector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/Injector.java index bcfee64f..2c836da1 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/Injector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/Injector.java @@ -4,7 +4,6 @@ import org.bukkit.entity.Player; import com.comphenix.protocol.PacketType.Protocol; import com.comphenix.protocol.events.NetworkMarker; -import com.comphenix.protocol.events.PacketEvent; /** * Represents an injected client connection. @@ -46,20 +45,6 @@ interface Injector { */ public abstract Protocol getCurrentProtocol(); - /** - * Undo the ignore status of a packet. - * @param packet - the packet. - * @return TRUE if the ignore status was undone, FALSE otherwise. - */ - public abstract boolean unignorePacket(Object packet); - - /** - * Ignore the given packet. - * @param packet - the packet to ignore. - * @return TRUE if it was ignored, FALSE if it already is ignored. - */ - public abstract boolean ignorePacket(Object packet); - /** * Retrieve the network marker associated with a given packet. * @param packet - the packet. @@ -74,13 +59,6 @@ interface Injector { */ public abstract void saveMarker(Object packet, NetworkMarker marker); - /** - * Associate a given network marker with a packet event. - * @param marker - the marker. - * @param event - the packet event - */ - public abstract void saveEvent(NetworkMarker marker, PacketEvent event); - /** * Retrieve the current player or temporary player associated with the injector. * @return The current player. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java index b39bf798..7d9ab741 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java @@ -227,48 +227,27 @@ public class NettyProtocolInjector implements ChannelListener { } @Override - public Object onPacketSending(Injector injector, Object packet, NetworkMarker marker) { + public PacketEvent onPacketSending(Injector injector, Object packet, NetworkMarker marker) { Class clazz = packet.getClass(); if (sendingFilters.contains(clazz)) { - // Check for ignored packets - if (injector.unignorePacket(packet)) { - return packet; - } PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(clazz), packet); - PacketEvent event = packetQueued(container, injector.getPlayer()); - - if (!event.isCancelled()) { - injector.saveEvent(marker, event); - return event.getPacket().getHandle(); - } else { - return null; // Cancel - } + return packetQueued(container, injector.getPlayer()); } // Don't change anything - return packet; + return null; } @Override - public Object onPacketReceiving(Injector injector, Object packet, NetworkMarker marker) { + public PacketEvent onPacketReceiving(Injector injector, Object packet, NetworkMarker marker) { Class clazz = packet.getClass(); if (reveivedFilters.contains(clazz)) { - // Check for ignored packets - if (injector.unignorePacket(packet)) { - return packet; - } PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(clazz), packet); - PacketEvent event = packetReceived(container, injector.getPlayer(), marker); - - if (!event.isCancelled()) { - return event.getPacket().getHandle(); - } else { - return null; // Cancel - } + return packetReceived(container, injector.getPlayer(), marker); } // Don't change anything - return packet; + return null; } @Override