fix(lwip): Add early out in NetworkUDP::parsePacket() when socket has no data
#10075
Conversation
|
|
👋 Hello nitz, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
…as no data Previously, `NetworkUDP::parsePacket()` would take the time to allocate a 1460 byte buffer to call `recvfrom()` with, immediately freeing it if there was no data read. This change has it check if there is available data via `ioctl()` with `FIONREAD` first, saving the allocation and thus significantly increasing performance in no data situations.
nitz
force-pushed
the
networkudp-parse-packet-perf-improvements
branch
from
4871631 to
47993cd
Compare
|
thank you for the PR @nitz, we will review it during next week 💯 |
libraries/Network/src/NetworkUdp.cpp
Outdated
| @@ -297,6 +297,13 @@ int NetworkUDP::parsePacket() { | |||
| struct sockaddr_storage si_other_storage; // enough storage for v4 and v6 | |||
| socklen_t slen = sizeof(sockaddr_storage); | |||
| int len; | |||
There was a problem hiding this comment.
here len should be set to 0, else it might not be initialized and might not be 0 for the next check
There was a problem hiding this comment.
Done! I've clearly spent too much time in C# lately, where "out" variables are guaranteed to be assigned 😂
me-no-dev
added
Status: Pending Merge
Test Results 56 files - 61 56 suites - 61 4m 52s ⏱️ - 1h 19m 41s Results for commit 0e5f420. ± Comparison against base commit 0fa4aa6. This pull request removes 9 tests.♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description of Change
Previously,
NetworkUDP::parsePacket()would take the time to allocate a 1460 byte buffer to callrecvfrom()with, immediately freeing it if there was no data read. This is an waste of cycles and provides no benefit.This change has
parsePacket()check if there is available data viaioctl()withFIONREADfirst, saving the allocation and thus significantly increasing performance in no data situations.Tests scenarios
Tested on Arduino-esp32 core v2.0.3 on an ESP32-C3 dev board (XAIO_ESP32C3) by sending UDP messages from my development machine.
In a near-empty sketch, invoking
parsePacket()everyloop()call, I was executing about 20,000 calls per second. After the change, the performance was closer to 35,000 calls/second.Related links
I did not find any related issues. However, as I was making this change, I did note the
parsePacket()'s allocation behavior overall. It seems excessive to allocate 1460 bytes every single call to it, here:arduino-esp32/libraries/Network/src/NetworkUdp.cpp
Line 300 in 0fa4aa6
Even with the change in this PR, that buffer is allocated, and then written into a second buffer,
rx_buffer, which is allocated at the end of the function, here:arduino-esp32/libraries/Network/src/NetworkUdp.cpp
Lines 335 to 340 in 0fa4aa6
It seems like it would be a better option to at least attempt to only allocate a single time; or to consider moving the temporary buffer allocation to be static (or potentially stack allocated.)