WLED icon indicating copy to clipboard operation
WLED copied to clipboard

LockedJsonResponse: Release early if possible

Open willmmiles opened this issue 2 years ago • 3 comments

Release the json buffer lock as soon as we've finished serializing. This should slightly reduce the number of lock collisions by releasing sooner - the response class isn't destructed until after the last packet is ack'd.

willmmiles avatar Feb 15 '24 00:02 willmmiles

Apologies for the incorrect initial push, I grabbed the wrong version of this commit.

willmmiles avatar Feb 15 '24 00:02 willmmiles

One question: Is it really necessary to release buffer lock early? And: What's the time difference? I wouldn't expect it to be more than a few 100us earlier.

blazoncek avatar Feb 19 '24 21:02 blazoncek

I don't have exact numbers, but it's a couple of round trip times -- it can be 10s of ms. (Data goes out, ack comes back, fin goes out, fin-ack comes back). If a second request comes in and the lock is contended, then we hit a minimum of 1s retry delay, which really hurts. (This is not the same as the lock delay - a 503 response has a minimum 1s backoff delay.) This was happening routinely during my local tests when a browser tried to send out multiple requests in parallel during initial page load.

The biggest impact of this patch is on requests that can be served in a single outbound packet buffer (eg <1k); in that case, the lock is released before the callback even exits, as the first packet is sent in the send call. This eliminates the possiblity of contention altogether, dramatically improving responsiveness.

willmmiles avatar Feb 19 '24 22:02 willmmiles