SFML community forums
Help => Network => Topic started by: P@u1 on October 22, 2011, 03:56:01 pm
-
Hi everyone,
I just read some of the network source code and recognized a possible problem.
TcpSocket::Send(Packet& packet) looks like this:
TcpSocket::Send(Packet& packet)
//...
// First send the packet size
Uint32 packetSize = htonl(static_cast<unsigned long>(size));
Status status = Send(reinterpret_cast<const char*>(&packetSize), sizeof(packetSize));
//...
// Send the packet data
if (packetSize > 0)
{
return Send(data, size);
}
//...
And you also disable nagle-algorithm, so that if I understand it right every call to send directly sends a physical packet over the network.
You are calling send 2 times in a row with very low time between the calls.
If that really yields 2 phyiscal packets (and it should do as nagle algorithm is off) with tcp/ip-header then this is quite a big overhead, don't you think so?
-
That's right, but how could I avoid that? Concatenating all the data in a single buffer would require a dynamic memory allocation, which is even worse.
Anyway, it's a potential problem but its impact would have to be measured precisely before complicating the implementation ;)
-
I don't agree that a dynamic memory allocation is worse than using an additoinal packet.
Bandwith is VERY rare while cpu and ram are VERY fast.
I would sacrifice a lot of cycles to gain additional bandwith :-)
Maybe you could reuse one or more buffers (maybe hold them in a pool)?
-
Like I said, I won't do anything until someone proves me that he really has performances problems with this ;)
-
What kind of prove do you need?
If you are only talking about having perfomance problems with tcp at all, this would be no problem.
-
What kind of prove do you need?
Something like "I have a problem with that, look".
"There might be a problem with that" is not enough, especially if it requires making the implementation uglier and more complicated.
-
I did some tests:
int main(int argc, char** argv)
{
sf::TcpListener listener;
sf::Socket::Status status = listener.Listen(3456);
alwaysAssert(status == sf::Socket::Done);
sf::Thread thread([&]()
{
sf::TcpSocket socket;
sf::Socket::Status status = listener.Accept(socket);
alwaysAssert(status == sf::Socket::Done);
std::size_t receivedLen;
char buffer[10000];
while(true)
{
socket.Receive(buffer, sizeof(buffer), receivedLen);
}
});
thread.Launch();
sf::TcpSocket socket;
status = socket.Connect("127.0.0.1", 3456);
alwaysAssert(status == sf::Socket::Done);
sf::Packet toSend;
toSend << "hello";
const char * const msg = "1234hello";
unsigned int len = strlen(msg);
for(int i = 0; i < 20; ++i)
{
sf::Clock clock;
for(int i = 0; i < 100000; ++i)
{
status = socket.Send(toSend);
alwaysAssert(status == sf::Socket::Done);
}
unsigned int elapsed = clock.GetElapsedTime();
cout << "Duration with packet: " << elapsed << " ms" << endl;
//give the network time to stabilize
sf::Sleep(10000);
clock.Reset();
for(int i = 0; i < 100000; ++i)
{
status = socket.Send(msg, len);
alwaysAssert(status == sf::Socket::Done);
}
elapsed = clock.GetElapsedTime();
cout << "Duration without packet: " << elapsed << " ms" << endl;
//give the network time to stabilize
sf::Sleep(10000);
}
thread.Terminate();
return 0;
}
I forwarded the output to a textfile and started it in release mode.
The output is this:
Duration with packet: 17732 ms
Duration without packet: 8257 ms
Duration with packet: 17495 ms
Duration without packet: 9239 ms
Duration with packet: 17827 ms
Duration without packet: 8324 ms
Duration with packet: 16481 ms
Duration without packet: 8103 ms
Duration with packet: 16720 ms
Duration without packet: 8155 ms
Duration with packet: 16290 ms
Duration without packet: 8158 ms
Duration with packet: 16321 ms
Duration without packet: 8132 ms
Duration with packet: 16337 ms
Duration without packet: 8162 ms
Duration with packet: 16294 ms
Duration without packet: 8168 ms
Duration with packet: 16403 ms
Duration without packet: 8187 ms
Duration with packet: 16337 ms
Duration without packet: 8132 ms
Duration with packet: 16388 ms
Duration without packet: 8074 ms
Duration with packet: 16714 ms
Duration without packet: 8167 ms
Duration with packet: 16321 ms
Duration without packet: 8533 ms
Duration with packet: 16336 ms
Duration without packet: 8184 ms
Duration with packet: 16331 ms
Duration without packet: 8140 ms
Duration with packet: 16310 ms
Duration without packet: 8157 ms
Duration with packet: 16260 ms
Duration without packet: 8357 ms
Duration with packet: 16418 ms
Duration without packet: 8129 ms
Duration with packet: 16191 ms
Duration without packet: 8080 ms
This shows that when testing locally with very small messages, using sf::Packet takes about twice the time then sending without sf::Packet.
I'm quite sure that it's because of 2 packets being sent instead of one.
Does this convince you, or do I need to do more? ^^
Here is also a possible solution:
When sending a packet you first make sure that the vector is large enough to store 4 additional bytes (for the size).
Then you use std::memmov to move the data 4 bytes to the right.
Then you write the size at the first 4 bytes.
Then you use the Send overload which takes raw bytes.
Then you use memmove again to revert the change.
I have 2 more questions:
- Why are you always using resize when something is written to a packet? If the standard-library implementation allocates exactly the size you request and no more then you need to reallocate for every write. Maybe it would be better to allocate more than is needed.
- When having an non-blocking socket and doing a sent, must the buffer which is sent then be unchanged until the operation is completed?
For example if I do:
char buffer[1024];
socket.Send(buffer, sizeof(buffer));
buffer[1023] = 'a';
Can the change to the buffer then affect what actually is sent?
With boost::asio this can happen, but it also tells you when the operation is completed.
-
Hmm... ok, you should add an issue to the task tracker, and maybe I'll take a look at it for SFML 2.1 :P
Why are you always using resize when something is written to a packet? If the standard-library implementation allocates exactly the size you request and no more then you need to reallocate for every write. Maybe it would be better to allocate more than is needed.
I don't optimize when there's no need to. Do another benchmark if you want me to change this ;)
When having an non-blocking socket and doing a sent, must the buffer which is sent then be unchanged until the operation is completed?
The Send function is blocking even if the socket is not. There's no asynchronous function in SFML, which would do something in the background and notify you when it's finished.
-
Concatenating all the data in a single buffer would require a dynamic memory allocation, which is even worse.
I don't optimize when there's no need to. Do another benchmark if you want me to change this ;)
First you a fearing dynamic allocations and then you don't care :-)
But it's not a problem. I never had problems with this, it was just a suggestion.
Hmm... ok, you should add an issue to the task tracker, and maybe I'll take a look at it for SFML 2.1 :P
Done.
-
That's right, but how could I avoid that? Concatenating all the data in a single buffer would require a dynamic memory allocation, which is even worse.
Or you can always keep an Uint32 allocated in the buffer right before the data and write the packet size to it when it's being sent.
Okay it introduces a bit of pointer arithmetics but nothing aweful, really.
This way you don't do any dynamic allocations, the only downside is that every Packet object is bigger of an int. (I don't think this is a big concern)
I just submitted a pull request on GitHub with my fixes. See how you're happy with it, I'd really need this implemented as well.
-
Or you can always keep an Uint32 allocated in the buffer right before the data and write the packet size to it when it's being sent.
This is kind of ugly too because the packet directly manages an implementation detail of the TCP socket.
This tight coupling between classes in the implementation details makes the code more difficult to maintain.
-
Yeah, I have to admit it's an hack-ish way to do things. Tho, that's how major networking libraries implement it (ACE (http://www.cs.wustl.edu/~schmidt/ACE.html) is the best library I'm aware of).
They used the have only one class, but now it's cut into 3 parts, very dependant on each-others. You might hate it, but it's a HUGE optimisation.
-----------------
You're talented with graphics, I give you that. But I've been working with sockets and networks for a long time too. This is really something you would want fixed if you were to code a game with SFML.
The only other ways I can think is having TcpSocket allocate a copy of the packet and prepend the size before copying the actual packet,
or
leave it bugged (please don't).
-
My message was just an immediate comment to your proposed solution, it doesn't mean that I have a better one, or that I prefer to leave the code bugged ;)
This will probably be the best solution, but I can't blindly merge your pull request without first thinking about this problem myself.
I'm currently very busy with the new graphics API, and after that I plan to release SFML 2.0, that's why this issue is tagged "2.x" ;)
-
Well it doesn't break the API anyway so I can use the network library for now and hope for a patch in the future :)
I just wanted to help out, it seems like there's just so many things going on this project at the same time.
I can predict a bright future for SFML. So far everything looks well though out, issues are considered one by one, it's just.. wow.
When I'm done with my game, I'll definitively donate to SFML and pay you a beer or something ahahah xD
Cheers!
-
Hehe, thanks :)
I've been stuck for a long time on the new graphics API, but now that it's almost done I'll be able to get back to a normal workflow, and start to work on all the smaller 2.x tasks.
-
One more thing to consider even with the current implementation:
If somebody sends a size of 0xFFFFFFFF to the server but no data,
the server will wait forever on the TcpSocket::Receive(packet) and thus block any communications with other clients until data is fully received.
This is quite a huge flaw in my opinion. What I did with my previous server, every connections created a Client object with a buffer. When a packet was received, we would read as much data as we could and push it to the client's buffer. Once there's enough bytes received, it was processed.
So if Receive is really blocking (and I think it is), that's a problem for server using a selector. In a threaded environment it'd be fine... but again, not for selector servers.
-
If somebody sends a size of 0xFFFFFFFF to the server but no data
Anybody can send any corrupt data. Packet size is just one special case. Packets are not at all made for safety, if someone sends a fake packet full of crap from its own client then there's nothing you can do against it.
SFML is a simple library, if you need a robust one with a lot of checks and tricks to ensure data validity, then you're using the wrong library ;)
When a packet was received, we would read as much data as we could and push it to the client's buffer. Once there's enough bytes received, it was processed.
That's good in an asynchronous environment -- you do stuff in the background, and notify the user when everything's done. But SFML is at a lower level: when user calls Receive he expects the whole packet to be filled when the function returns.
-
I think you misunderstood my post.
Corrupted data isn't a big deal. SFML doesn't have to do thoses checks, it's our job. For example: If the player wants to move to a coordinates that doesn't exist, we have to check that. When he pickup an item ID, interact with something.. those are integers we can validate ourselves with the game mechanics.
The problem comes from the fact SFML only provides a blocking Receive() function. With this API, you can't retrieve partial data and build a network manager on top. This results in a poor system where you can't control the server flow anymore! (When using a socket selector) Immagine a hacker send to your server a packet size without the data... ALL CONNECTIONS are interrupted because of Receive() waiting for more bytes it'll never receive...
---
Anyway, I think I'll use SFML for the graphics and ENet for the network. Thanks god you divided your project into smaller libraries so when can cherry-pick what we need ;)
-
Immagine a kid give to SFML a packet size he doesn't intend to send at all...
That's not supposed to happen, so SFML doesn't handle it. Like I said, SFML packets can easily be hacked with fake data, it's not meant to be a robust mechanism.
The problem comes from the fact SFML only provides a blocking Receive() function. You can't retrieve partial data and build a network manager on top.
It's not a problem, rather a design choice. Packets are made so that you don't have to worry about message boundaries. If you want to, then send/receive raw bytes, not packets.
I understand your conern, but what you want is incompatible with sf::Packet. But you can still use Send(const char*) and/or non-blocking sockets to build your own network manager.
-
Oh wait. If we exclude sf::Packet, isn't Receive(const char*,size,...) blocking too? I don't need the fancy packets, I just want to read/write raw data through SFML in a way that isn't blocking..
I might owe you some excuses ahah xD Annnnd a beer if I remember right ;)
-
Socket can be made non-blocking
socket.SetBlocking(false);
-
. . . right. Okay I'm sorry. I'll implement the said network manager now then x]
Oh well at least its a good warning to anybody who uses blocking sockets + sf::Packet for production. A custom implementation is better.
-
You can even use sf::Packet to encode/decode the data (to get rid of endianness and types size problems), but send/receive their contents yourself.