Skip to content

Faster GlobalBufferAsyncJsonResponse#3748

Closed
willmmiles wants to merge 2 commits intowled:mainfrom
willmmiles:global-json-fix-v3
Closed

Faster GlobalBufferAsyncJsonResponse#3748
willmmiles wants to merge 2 commits intowled:mainfrom
willmmiles:global-json-fix-v3

Conversation

@willmmiles
Copy link
Copy Markdown
Member

For early review of the approach, this is intended to be better version of #3743, but there are still some TODOs and bugs to work out.

Rather than hold the JSON buffer lock until the entire request is done, serialize the request completely on when the network stack fills the first outgoing packet buffer. We split the serialized response data between the packet buffer and heap-allocated secondary buffer (if one is needed). After serialization, release the JSON document for reuse.

The key insights in this approach is that

  • many responses fit entirely in the packet buffer, and so require no extra memory and can be released quickly;
  • if there is overflow, it is typically small and sent quickly; and
  • the first outbound packet buffer is usually filled in request->send() before serveJson even returns, so the scoping of the JSON buffer lock is so small it can't collide.

GlobalBufferAsyncJsonResponse was still trying to init AsyncJsonResponse
with the shared document, even when it failed to acquire the lock.
This was still corrupting memory and causing crashes.

Possibly fixes wled#3690, wled#3685, and other 0.14.1 issues.
Rather than hold the lock through the entire response process, serialize
the result completely when the initial packet buffer is filled, and then
release the lock.   Allocate only enough extra memory to hold the part
of the serialized response that can't go directly to the output packet
buffer.
@blazoncek
Copy link
Copy Markdown
Contributor

blazoncek commented Feb 7, 2024

Since you are rewriting AsyncJsonResponse class, why not utilise PSRAMDynamicJsonDocument so that PSRAM can be used? In such case (when PSRAM exists) everything is much simpler.
Of course there would need to be some fallback to use static buffer as well.

@willmmiles
Copy link
Copy Markdown
Member Author

This implementation uses the global JSON document, so it'll follow the same PSRAM behavior of the global document. If you're suggesting that we could store the serialized JSON in PSRAM too: yes, that's probably a good call. I should likely pick up a PSRAM board for testing.

@willmmiles
Copy link
Copy Markdown
Member Author

I did consider outright replacing AsyncJsonResponse; that might be a good approach for the ArduinoJson v7 WIP branch. AsyncJsonResponse is, I think, operating on some assumptions that don't hold up with ArduinoJson v6 or v7 (can't speak for earlier versions). The JsonDocument buffer is typically larger than the serialized JSON -- much larger if the strings are short. So unless the caller plans on sharing one JsonDocument to many responses, it ends up consuming a ton of RAM (holding the JsonDocument in scope) and CPU as it re-serializes completely for every reply packet. For our use cases here where every web request wants to generate its own JSON reply, AsyncJsonDocument is decidedly suboptimal.

Annoyingly the other immediately available approach -- serialize to a String and then use AsyncBasicResponse -- is worse, since it requires holding the JsonDocument and TWO complete copies of the serialized reply all in RAM together during construction, since AsyncBasicResponse can't move-construct its reply buffer, either. :(

@blazoncek
Copy link
Copy Markdown
Contributor

I propose to close this PR as guarded & locked JSON response is in place via #3760 and 4f42a17

@willmmiles
Copy link
Copy Markdown
Member Author

This PR isn't the same as the other locking fixes: this one is about storing the serialized JSON output in the web response, but using a chain of small buffers instead of serializing to a large contiguous buffer, to allow faster memory recovery and be more resistant to fragmentation. The major advantage is better support for parallel web requests, particularly for ESP32s which are not so RAM limited, but implemented with an eye on keeping ESP8266es stable. I'd opened this draft PR to get your feedback early on the overall idea of serializing early rather than holding the lock.

Unfortunately I think the specific patch here is more confusing than anything else. As an artifact of my particular development path, the buffering changes were mixed in with the locking logic. Further, as I've spent more time with the web server code, I've learned that there's basically no value in doing early serialization in the response object. The first output packet will be generated immediately after the handler function is called. So if we do want to do early serialization, it can be done much more clearly directly in the handler function. (In fact, doing so would eliminate the need for holding the lock in the response object altogether!)

All of this said: I agree we should close this PR - it can be considered obsolete as I'm implementing buffer list support in the web server repo. I'll open PRs with better targeted changes to WLED once that's ready.

@willmmiles willmmiles closed this Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants