Speed up upload by a factor of 17

[MitchBradley]

Uploads are very slow because of an unnecessary "client.connected()" check in _uploadReadByte().

Here is what happens:
client.connected() is called for every byte read. WiFiClient::connected() calls recv(fd(), &dummy, 0, MSG_DONTWAIT); which takes a relatively long time, so the optimized path of returning a buffered byte via client.read() is effectively nullified.

Removing the one line changed the upload speed for a 2 MB file (discarding the received data) from 22 KB/sec (before) to 367 KB/sec (after).

The change is safe in the face of disconnects because client.read(), when it no longer has buffered data, calls (WiFiClient) fillBuffer(), which calls recv(), so the disconnection will be detected in due course.

[bdring]

I have done some basic testing on this and can confirm a huge improvement in upload speeds.

[Jason2866]

Tested with Tasmota32 firmware upload by file function. Upload time reduced from 82 sec. to 33 sec.

[lyusupov]

Commits history says that the assertion was added as a response on #2914 in June of 2019.
So we need to double check to make sure that this old issue will not come up again.

[sansillusion]

Mabe an alternative would be to check every 16 byte or whatever would be proper ?

[lbernstone]

I think the real concern here is that the socket could disappear, and then you will get a fatal error (maybe there is a simpler check for that). I'll try some tests to generate failures. The speedup is worth the occasional non-fatal error (which should just cause a resend).

[MitchBradley]

The "socket disappearance" case already works in my test setup. I will first describe the code path for that case, and then describe my test setup.

When the socket disappears:

Calls to _uploadReadByte() continue to succeed, returning correct data, while there is data in the WiFiClient rxbuffer. This is the fast path where client.read() checks a few memory variables and then return a buffered byte.

When all of the buffered data has been returned, the next call to client.read invokes fillBuffer() via this line at the top of WiFiClient.cpp :: read():

    if (!dst || !len || (_pos == _fill && !fillBuffer())){    // fillBuffer() is called because _pos == _fill

fillBuffer() then returns 0 via this clause:

            if(!_buffer || _size <= _fill || !r_available()) {
                return 0;
            }

because, in r_available(), lwip_ioctl_r(_fd_ FIONREAD, &count) returns <0 .

Since fillBuffer() returns 0, client.read() returns -1, causing an entry into _uploadReadByte()'s for(;;) loop.

The while(!timedOut && !client.available() && client.connected()){ loop exits immediately because client.connected() now returns false, but the loop continues to retry client.read(), eventually exiting when timedOut.

I verified this analysis by adding a lot of Serial.println() calls to trace the code path.

The only way I can see for this to hang per #2914 is if the timeout has been set to a very large value, so it appears to hang but the timeout has not been reached.

There is a simple fix that involves moving the check for !client.connected inside the loop. With that check, a disconnect will abort the upload immediately, without waiting for a timeout. I will revise the PR accordingly.

[MitchBradley]

My test setup involves using Grbl_Esp32. It has a WebUI component. WebUI lets you upload files to an SD Card attached to the ESP32. To test the upload speed, I upload a large file to SD and time it with a stopwatch app. To test the raw upload speed, I modified the Grbl_ESP32 source code so that it discards the data instead of writing to the SD file. To test the client disconnect behavior, I close the browser window that is running WebUI while the upload is in progress, observing the disconnect via messages displayed on the ESP32 serial console.

[me-no-dev]

Done :)