CODEWITHSUNDEEP
javasocketsbuffering
Reacen
Reacen

Reputation: 2360

Reading socket one byte a time, how to change this, optimisation

I need some help about optimisation. I am trying to improve this open-source game server made with JAVA. Each player has its own thread, and each thread goes something like this:

BufferedReader _in = new BufferedReader(new InputStreamReader(_socket.getInputStream()));

String packet = "";
char charCur[] = new char[1];

while(_in.read(charCur, 0, 1)!=-1)
{
    if (charCur[0] != '\u0000' && charCur[0] != '\n' && charCur[0] != '\r')
    {
        packet += charCur[0];

    }else if(!packet.isEmpty())
    {
        parsePlayerPacket(packet);
        packet = "";
    }
}

I have been told so many times that this code is stupid, and I agree because when profiling it I see that reading each byte and appending it using packet += "" is just stupid and slow.

I want to improve this but I don't know how.. I'm sure I can find something, but I'm afraid it will be even slower than this, because I have to split packets based on the '\u0000', '\n', or '\r' to parse them. And I know that splitting 3 times is verry slow.

Can someone give me an idea? Or a piece of code for this? It will make my day.

If you're going to explain, please, please use verry simple words, with code examples, I'm just a JAVA beginner. Thank's

Upvotes: 2

Views: 2764

Answers (3)

Stephen C
Stephen C

Reputation: 718798

There is no significant performance issue with reading from a BufferedReader either in large chunks, or even one character at a time.

Unless your profiling has identified the BufferedReader.read() method as a specific hotspot in your code, the best thing you can do is make the code simple and readable, and not spend time optimizing it.

For your particular case:

  • yes that code is a bit lame, but
  • no it is unlikely to make a lot of difference from the performance perspective.

The real performance bottleneck is most likely the network itself. There are application level things that you can do to address this, but ultimately you can only send / receive data at a rate that the end-to-end network connection will support.

My profiling result is saying that it's coming from: BufferedReader.read(). What does it mean really?

Are you sure that the time is really being spent in the Socket's read method? If it is, then the real issue is that your application threads are spending lots of time waiting for network packets to arrive. If that is the case, then the only thing you could do is to reduce the number of client and server side flushes so that the network doesn't have to deal with so many small packets. Depending on your application, that may be infeasible.

I'd write your code as follows:

BufferedReader _in = new BufferedReader(
        new InputStreamReader(_socket.getInputStream()));

StringBuilder packet = new StringBuilder();
int ch;

while ((ch = _in.read()) != 1) {
    if (ch != '\u0000' && ch != '\n' && ch != '\r') {
        packet.append((char) ch);
    } else if (!packet.isEmpty()) {
        parsePlayerPacket(packet.toString());
        packet = new StringBuilder();
    }
}

But I don't think it will make much difference to the performance ... unless the "packets" are typically hundreds of characters long. (The real point of my tweaks is to reduce the number of temporary strings that are created while reading a packet. I don't think that there's a simple way to make it spend less real time in the read calls.)

Upvotes: 2

Jon Lin
Jon Lin

Reputation: 143886

Perhaps you should look into the readLine() method of BufferedReader. Looks like you're reading Strings, calling BufferedReader.readLine() gives you the next line (sans the newline/linefeed).

Something like this:

String packet = _in.readLine();
while(packet!=null) {
    parsePlayerPacket(packet);
    packet = _in.readLine();
}

Just like you're implementation, readLine() will block until either the stream is closed or there's a newline/linefeed.

EDIT: yeah, this isn't going to split '\0'. You're best bet is probably a PushbackReader, read in some buffer of chars (like David Oliván Ubieto suggests)

PushbackReader _in = new PushbackReader(new InputStreamReader(_socket.getInputStream()));
StringBuilder packet = new StringBuilder();
char[] buffer = new char[1024];
// read in as much as we can
int bytesRead = _in.read(buffer);

while(bytesRead > 0) {
    boolean process = false;
    int index = 0;
    // see if what we've read contains a delimiter
    for(index = 0;index<bytesRead;index++) {
        if(buffer[index]=='\n' ||
           buffer[index]=='\r' ||
           buffer[index]=='\u0000') {
            process = true;
            break;
        }
    }
    if(process) {
         // got a delimiter, process entire packet and push back the stuff we don't care about
         _in.unread(buffer,index+1,bytesRead-(index+1));  // we don't want to push back our delimiter
         packet.append(buffer,0,index);
         parsePlayerPacket(packet);
         packet = new StringBuilder();
    }
    else {
         // no delimiter, append to current packet and read some more
         packet.append(buffer,0,bytesRead);
    }
    bytesRead = _in.read(buffer);
}

I didn't debug that, but you get the idea.

Note that using String.split('\u0000') has the problem where a packet ending with '\u0000' won't get processed until a newline/linefeed is sent across the stream. Since you're writing some kind of game, I assume it's important to process an incoming packet as soon as you get it.

Upvotes: 1

David Oliv&#225;n
David Oliv&#225;n

Reputation: 2725

Read as many bytes as you can using a large buffer (1K). Check if the "terminator" is found ('\u0000', '\n', '\r'). If not, copy to a temporal buffer (larger than used to read socket), read again and copy to the temporal buffer until "terminator" found. When you have all the necessary bytes, copy the temporal buffer to any "final" buffer and process it. The remaining bytes should be considered as the "next" message and copied to the start of the temporal buffer.

Upvotes: 0

Related Questions