httpClient: enormous performance issues when sending body.

[sandwoodK]

Nim Version

Nim Compiler Version 2.2.0 [Linux: amd64]
Compiled at 2024-10-02
Copyright (c) 2006-2024 by Andreas Rumpf

git hash: 78983f1
active boot switches: -d:release

Description

Basically when using request proc, performance are catastrophic as soon as there is a body (even if the body is extra small).

This can be reproduced quite easily.
Running both client and backend on the same machine.(bottleneck is not network).
Tested with 3 various backends (one using asynchttpserver, one using mummy, one implemented in C++ Boost/Beast), all tested where showing the same behaviour.
I have also eliminated all other possibles bottleneck..

See here (6789 is the port)

=> a run with 1000 requests with a body size of 10 bytes

~/nim_stuff/client_server$ ./mockclient 127.0.0.1 6789 / --clients=1 --requests=1000 --random_body_size=10
[ elapsed ] 41.004 s
[ transactions ] 1000 
[ transaction_rate ] 24 req/s
[ sent body bytes ] 10000 bytes
[ sent headers bytes ] 0 bytes
[ received body bytes ] 6893 bytes
[ received headers bytes ] 2000 bytes

=> a run with 1000 requests with no body

~/nim_stuff/client_server$ ./mockclient 127.0.0.1 6789 / --clients=1 --requests=1000
[ elapsed ] 0.100 s
[ transactions ] 1000 
[ transaction_rate ] 9967 req/s
[ sent body bytes ] 0 bytes
[ sent headers bytes ] 0 bytes
[ received body bytes ] 6893 bytes
[ received headers bytes ] 2000 bytes

Current Output

Expected Output

Known Workarounds

No response

Additional Information

I have 'hacked' the httpclient.nim code and figured out that changing the code in requestAux to perform only ONE await client.socket.send instead of two brings back performances to what we can expect

Something like that

  let headerString = generateHeaders(url, httpMethod, newHeaders,
                                     client.proxy)
  if (body.len> 0):
    await client.socket.send(headerString & body)
  else: 
    await client.socket.send(headerString)

  if data.len > 0:
    var buffer: string = ""
    for i, entry in multipart.content:
      buffer.add data[i]
      if not entry.isFile: continue
      if buffer.len > 0:
        await client.socket.send(buffer)
        buffer.setLen(0)
      if entry.isStream:
        await client.socket.sendFile(entry)
      else:
        await client.socket.send(entry.content)
      buffer.add httpNewLine
    # send the rest and the last boundary
    await client.socket.send(buffer & data[^1])

now it's ok

:~/nim_stuff/client_server$ ./mockclient 127.0.0.1 6789 / --clients=1 --requests=1000 --random_body_size=10
[ elapsed ] 0.100 s
[ transactions ] 1000 
[ transaction_rate ] 9970 req/s
[ sent body bytes ] 10000 bytes
[ sent headers bytes ] 0 bytes
[ received body bytes ] 6893 bytes
[ received headers bytes ] 2000 bytes

So. Not sure where the issue is really, maybe in the async machinery ?

[Araq]

So. Not sure where the issue is really, maybe in the async machinery ?

Me neither but can you please create a PR with this fix?

[sandwoodK]

Sure I can create a PR. But is it really the right fix ?
I feel uncomfortable with the fact that I don't understand why having only one async call instead of two drastically improves the performances
(or better say, I would not expect that having two async call drastically reduce the performance).
Plus it would only 'fix' the case of a Http request with a 'blob' body, (i.e not the multipart/form-data case which must be done in several async because each part can be a file stream.)

[nitely]

Add:

client.socket.setSockOpt(OptNoDelay, true, level = IPPROTO_TCP.cint)

after this line. I verified that fixes the issue or at least one issue... hard to say because there was no code provided :)

Edit: better do it after the else so the non-async client gets the improvement too.
Edit2: probably good idea to set it for the server as well here
Edit3: @sandwoodK if you want to learn why this fixes it (assuming it does for you) read this
Edit4: also delayed ack could be disabled instead of nagle's. However, 1. delayed ack must be disabled again after every recv, and 2. it's not portable, it cannot be disabled in some platforms (FreeBSD, windows, and macos apparently). Here there is an interesting read about the two too.

[sandwoodK]

@nitely : thank you very much, you were completely right in the analysis.

Setting the option after the first call to request function in user code (not in httpClient code) also work.
Got a segfault if done before but after the creation of the connection.
The connected state of the httpClient is not public so there is no way to check that the getSocket function return something usable.

Can we / shall we make this fix on the std/httpClient ? (portability, potential drawbacks in other cases ?)

[sandwoodK]

Some comparaison to existing library :
boost/beast does not set it by default.
boost/asio neither but examples show that it should be set

 * boost::asio::connect(sock.lowest_layer(), resolver.resolve(query));
 * sock.lowest_layer().set_option(tcp::no_delay(true));

On the other side, POCO HTTPSession (which is higher lever, as nim httpClient could be) set it unconditionally

void HTTPSession::connect(const SocketAddress& address)
{
	_socket.connect(address, _connectionTimeout);
	_socket.setReceiveTimeout(_receiveTimeout);
	_socket.setSendTimeout(_sendTimeout);
	if (address.family() != SocketAddress::UNIX_LOCAL)
		_socket.setNoDelay(true);
	// There may be leftover data from a previous (failed) request in the buffer,
	// so we clear it.
	_pCurrent = _pEnd = _pBuffer;
}

So i presume we should do it.

[nitely]

Not sure, I think we could add a way to enable/disable tcp_nodelay in newAsyncHttpServer similar to reuseAddr/Port (here), same for newAsyncHttpClient and newHttpClient. The question is if it should be enabled by default or not. Maybe disabled by default, but enable it in all examples.

Also, you probably want to add your initial fix as well.

Also note I'm just a nim user :), core devs may have a different idea.