ClientConnection uses too much heap when streaming files

[joelucid]

It should be possible to stream arbitrary large files to a ClientContext. However this is done via a BufferedStreamDataSource which allocates a buffer in RAM to keep the whole memory chunk requested in get_buffer. These requests can ask for > 2800 bytes at a time.

To preserve memory the size of the chunk requested should be limited. The following patch achieves that:

diff --git a/libraries/ESP8266WiFi/src/include/ClientContext.h b/libraries/ESP8266WiFi/src/include/ClientContext.h
index d2ee090..df12760 100644
--- a/libraries/ESP8266WiFi/src/include/ClientContext.h
+++ b/libraries/ESP8266WiFi/src/include/ClientContext.h
@@ -346,6 +346,7 @@ protected:
can_send = 0;
}
size_t will_send = (can_send < left) ? can_send : left;

•   if( will_send > 256 ) will_send = 256;
     if (will_send) {
         const uint8_t* buf = _datasource->get_buffer(will_send);
         err_t err = tcp_write(_pcb, buf, will_send, TCP_WRITE_FLAG_COPY);
  

[igrr]

Actually it will not pull the whole file into memory. It should never allocate more than 2 * TCP_MSS bytes at a time (that's 2920 bytes).

[joelucid]

I see. Still that's quite a bit of memory for an embedded system. It caused my application to break. There's very little benefit to using a larger block size (see https://github.com/pellepl/spiffs/wiki/Performance-and-Optimizing) for the SPIFFS case.

[igrr]

This optimization is mainly for TCP throughput, not for SPIFFS performance. I'll see if there is a nice way to configure TCP window size individually for each WifiClient connection, so that you can dial down buffer size of the application requires that.

[joelucid]

Is that really a necessary optimization for TCP throughput? The tcp_write documentation has this to say:

• Write data for sending (but does not send it immediately).

• It waits in the expectation of more data being sent soon (as

• it can send them more efficiently by combining them together).

• To prompt the system to send data now, call tcp_output() after

• calling tcp_write().

So shouldn't one just write in smaller chunks until MSS is reached and then call tcp_output to flush?

I'll see if there is a nice way to configure TCP window size individually for each WifiClient connection, so that you can dial down buffer size of the application requires that.

I actually want both good throughput and tight memory use, so I'd hate to configure a smaller TCP window size.

[joelucid]

Like this:

diff --git a/esp8266/libraries/ESP8266WiFi/src/include/ClientContext.h b/esp8266/libraries/ESP8266WiFi/src/include/ClientContext.h
index df12760..75a9fa9 100644
--- a/esp8266/libraries/ESP8266WiFi/src/include/ClientContext.h
+++ b/esp8266/libraries/ESP8266WiFi/src/include/ClientContext.h
@@ -334,6 +334,8 @@ protected:
     }
 
 
+#define TCP_WRITE_CHUNK_SIZE 128
+       
     void _write_some()
     {
         if (!_datasource || !_pcb) {
@@ -346,16 +348,20 @@ protected:
             can_send = 0;
         }
         size_t will_send = (can_send < left) ? can_send : left;
-               if( will_send > 256 ) will_send = 256;
-        if (will_send) {
-            const uint8_t* buf = _datasource->get_buffer(will_send);
-            err_t err = tcp_write(_pcb, buf, will_send, TCP_WRITE_FLAG_COPY);
-            _datasource->release_buffer(buf, will_send);
+               bool did_write = false;
+               while( will_send ) {
+                       size_t next_chunk =
+                               will_send > TCP_WRITE_CHUNK_SIZE ? TCP_WRITE_CHUNK_SIZE : will_send;
+            const uint8_t* buf = _datasource->get_buffer(next_chunk);
+            err_t err = tcp_write(_pcb, buf, next_chunk, TCP_WRITE_FLAG_COPY);
+            _datasource->release_buffer(buf, next_chunk);
             if (err == ERR_OK) {
-                _written += will_send;
-                tcp_output(_pcb);
+                _written += next_chunk;
+                               did_write = true;
             }
+                       will_send -= next_chunk;
         }
+               if( did_write ) tcp_output(_pcb);
 
         if (!_datasource->available() || _noblock) {
             delete _datasource;

[igrr]

What throughput do you get with this change, compared to the version in the master branch?

[joelucid]

With a chunk size of 128 I get ~290 kb/s when curling a 1mb file. Chunk size 256 or the old code both achieve about 330kb/s.

[igrr]

Looks good, if you can put up a pull request with this change that would be great. Thanks.

[devyte]

@igrr @joelucid The code in the PR is merged into master already. Is there anything pending? Can this be closed?