tags:

views:

245

answers:

5

Hi have implemented simple file exchange over a client/server connection in c++. Works fine except for the one problem that its so damn slow. This is my code:

For sending the file:

int send_file(int fd)
{

char rec[10];
struct stat stat_buf;
fstat (fd, &stat_buf);  
int size=stat_buf.st_size;

while(size > 0)
{
    char buffer[1024];
    bzero(buffer,1024);
    bzero(rec,10);
    int n;
    if(size>=1024)
    {
        n=read(fd, buffer, 1024);

        // Send a chunk of data
        n=send(sockFile_, buffer, n, 0 );

        // Wait for an acknowledgement
        n = recv(sockFile_, rec, 10, 0 );
    }
    else // reamining file bytes
    {
        n=read(fd, buffer, size);
        buffer[size]='\0';
        send(sockFile_,buffer, n, 0 );
        n=recv(sockFile_, rec, 10, 0 ); // ack
    }

    size -= 1024;

}

// Send a completion string
int n = send(sockFile_, "COMP",strlen("COMP"), 0 );
char buf[10];
bzero(buf,10);
// Receive an acknowledgemnt
n = recv(sockFile_, buf, 10, 0 );

return(0);
}

And for receiving the file:

int receive_file(int size, const char* saveName)
{

ofstream outFile(saveName,ios::out|ios::binary|ios::app);


while(size > 0)
{       
    // buffer for storing incoming data
    char buf[1024];
    bzero(buf,1024);
    if(size>=1024)
    {

        // receive chunk of data
        n=recv(sockFile_, buf, 1024, 0 );

        // write chunk of data to disk
        outFile.write(buf,n);

        // send acknowledgement
        n = send(sockFile_, "OK", strlen("OK"), 0 );

    }
    else
    {
        n=recv(sockFile_, buf, size, 0 );
        buf[size]='\0';
        outFile.write(buf,n);
        n = send(sockFile_, "OK", strlen("OK"), 0 );
    }   

    size -= 1024;

}

outFile.close();

// Receive 'COMP' and send acknowledgement
// ---------------------------------------
char buf[10];
bzero(buf,10);
n = recv(sockFile_, buf, 10, 0 );
n = send(sockFile_,  "OK", strlen("OK"), 0 );
std::cout<<"File received..."<<std::endl;

return(0);
}

Now here are my initial thoughts: Perhaps the buffer is too small. I should therefore try increasing the size from I dunno, 1024 bytes (1KB) to 65536 (64KB) blocks, possibly. But this results in file corruption. Ok, so perhaps the code is also being slowed down by the need to receive an acknowledgement after each 1024 byte block of data has been sent, so why not remove them? Unfortunately this results in the blocks not arriving in the correct order and hence file corruption.

Perhaps I could split the file into chunks before hand and create multiple connections and send each chunk over its own threaded connection and then reassemble the chunks somehow in the receiver....

Any idea how I could make the file transfer process more efficient (faster)?

Thanks, Ben.

+5  A: 

You should certainly increase the buffer size, and if this causes corruption it is an error in your code, which you need to fix. Also, if you use a stream protocol (i.e. TCP/IP) the order and delivery of packets is guaranteed.

anon
Have increased size to 2048 bytes... Had heard that TCP/IP delivered in correct order, it was the size-=1024 bug in the receiver that was causing corruption. Has been changed to size-=n, now.
Ben
+6  A: 

My guess is that you are getting out of sync and some of your reads are less than 1024. It happens all the time with sockets. The "size -= 1024" statement should be "size -= n".

My guess is that n is sometimes less than 1024 from the recv().

Richard Pennington
Thanks, this was exactly it :-)
Ben
+1  A: 

Read this thread:

http://stackoverflow.com/questions/2014033/send-file-in-socket-programing-in-linux-with-c-c/2014093#2014093

Oh, and use sendfile POSIX command, here's an example to get you started: http://tldp.org/LDP/LGNET/91/misc/tranter/server.c.txt

Kornel Kisielewicz
+11  A: 

Skip the acknowledgement of buffers! You insert an artificial round trip (server->client+client->server) for probably each single packet.

This slows down the transfer.

You do not need this ack. You are using TCP, which gives you a reliable stream. Send the number of bytes, then send the whole file. Do not read after send and so on.

EDIT: As a second step, you should increase the buffer size. For internet transfer you can assume an MTU of 1500, so there will be space for a payload of 1452 bytes in each IP packet. This should be your minimal buffer size. Make it larger and let the operating system slice the buffers into packets for you. For LAN you have a much higher MTU.

frunsi
Thanks :-) Ack removed and together with Richard Pennington's comment, this all works fine now...
Ben
Ok, glad I could help. Please mark one answer as accepted :)
frunsi
Yep, waiting for an ack on each send would definitely throw away all the TCP performance features.
Nikolai N Fetissov
A: 

A couple of things.

1) You are reallocating the buffer each time you go through your while loop:

while(size > 0)
{
    char buf[1024];

You can pull it out of the while loop on both sides and you won't be dumping on your stack as much.

2) 1024 is a standard buffer size, and I wouldn't go much above 2048 because then the lower level TCP/IP stack will just have to break it up anyways.

3) If you really need speed, rather than waiting for a recv ack you could just add a packet number to each packet and then check them on the receiving end. This makes your receiving code a little more complex because it has to store packets that are out of order and put them in order. But then you wouldn't need an acknowledgement.

4) It's a little thing, but what if the file that you are sending has a size that is a multiple of 1024... Then you won't send the trailing '/0'. To fix that you just need to change your while to:

while (size >= 0)
JasonOfEarth
Most compilers will allocate buf once at function entry not at the loop entry point.
Richard Pennington
Thanks -- point 4 seems very significant...
Ben
Actually, can you calrify about the while (size >= 0). I changed it to that but then the loop would carry on infinitely, so I've changed it back to while(size > 0) again. Cheers.
Ben
Ahh... if you made my modification as well as the one above... where you subtracted n not 1024 then it would go on infinitely. Because you would get to 0 and stay there. Subtracting 1024 each time will end the loop.
JasonOfEarth