Assuming you are transferring up to 10 MB/s, is it a good idea to recycle DatagramPacket objects, instead of creating a new one every time a packet is to be sent?
I'm creating a LAN file-sync application that deals with files over 30 GB at times. The file sync application will transfer the files over 100Mbit wired LAN. I already have an anti-packet-loss system in place (which works flawlessly).
The program works fine but takes around 10% CPU usage, and since this is a background application, this is too much for me. Ideally it will be around 3% tops.
When profiling I discovered the garbage collector was going mental, activating every few seconds. I know object creation (when done in large quantities) gets heavy for Java, so now I'm trying to recycle as many objects and arrays as I can. Each packet containing file data has a size of 1450 bytes, meaning that transferring at 10 MB/s will be around 7,200 packets per second. I decided to start recycling these packets (i.e. when a packet is sent, the DatagramPacket object will added to a list, and after 5 seconds the DatagramPacket can be re-used). When a DatagramPacket is re-used, the method DatagramPacket.setData() is used to assign the data it is going to send.
Apart from sending packets containing file data, I also send small packets roughly every second to try and determine the ping of the connection. These ping packets have a size of 10 bytes.
After testing my application with the DatagramPacket recycle feature for roughly 30 minutes, strange errors start to pop up. One time the file being transferred got corrupted, and other times I get something I can't wrap my head around... Below is some of the code of my class. The integer length is only set through the applyData() method.
public class PacketToSend {
private int length;
private DatagramPacket packet;
...
public void applyData(byte[] newData) {
try {
length = newData.length;
packet.setData(newData, 0, length);
} catch(java.lang.IllegalArgumentException e) {
System.out.println("Arraylength = "+newData.length);
System.out.println("length value = "+length);
}
}
...
}
After about 20-40 minutes testing each time, I get an IllegalArgumentException, telling me that newData's size is 10, and the value of length is 1450, thus saying the length is illegal. How is this even possible? The variable length is not modified anywhere else but in this method and is set right before setData() is called! It's as though the DatagramPacket randomly switches to sending the ping data...
These errors only occurs when I enable my DatagramPacket recycling feature.
Mind you, after a packet is sent, it is placed on a list and will wait for 5 full seconds before it is re-used again. I'm wondering if the OS somehow has its finger in these packets, or perhaps some native code is manipulating the data.
There is only one thread in my program that sends packets, so this is not a threading or synchronization issue.
Hence my question: is it a good idea to recycle DatagramPacket objects, instead of creating a new one every time a packet is to be sent? Or am I playing with fire and things I should really leave alone?
length = newData.length; after calling
setData(newData, 0, newData.length);, which prevented the
IllegalArgumentException but I still encountered other errors, such
as connection loss. Regardless, the error mentioned above, according
to my knowledge of Java, simply should never occur, so I think something else is at work here.Is it safe to recycle DatagramPacket objects?
As far as I am aware or can determine, there is nothing inherently unsafe about reusing DatagramPacket instances.
On the other hand, the errors you describe only make sense if instances are shared between two or more threads, and accessing shared objects from multiple threads without proper synchronization is definitely unsafe. No amount of waiting substitutes for synchronization, so the strategy of imposing a 5-second delay before reuse is probably counterproductive -- it does not guarantee correct operation, yet it may cause your program to maintain more live objects than it really needs.
Without details of your program's architecture, we can speak only in generalities about what you can do to resolve the situation. At the most general level, the alternatives are to avoid sharing objects between threads and to access the shared objects in a thread-safe manner. However, any mechanism for thread-safe object sharing carries relatively significant overhead, and I'm inclined to think that performing 20 million thread-safe operations in the course of one file transfer will be far too costly to be acceptable. Your best bet, therefore, is to avoid sharing objects.
Creating new DatagramPackets at every need and not allowing them to escape the threads in which they were created is one way to achieve that. Since that's causing too much GC for you, the next logical step might be to maintain per-thread queues of reusable packets. You might use a ThreadLocal for that, but if each file transfer is managed by a single thread then you could also consider using per-file queues. Either way, be careful also about other sharing, such as of the data buffer arrays carried by the DatagramPackets (which might well be something else you could reuse).
Also, if you are careful not to share data between threads, then you should be able to do so without a reuse delay. Indeed, you might not need more than one DatagramPacket and one buffer array per thread. That could make your code not only more efficient but also simpler.