feat: opt-in ASYNCWEBSERVER_USE_PSRAM - allocate handler objects in PSRAM (ESP32) - #468
feat: opt-in ASYNCWEBSERVER_USE_PSRAM - allocate handler objects in PSRAM (ESP32)#468inF1704 wants to merge 1 commit into
Conversation
mathieucarbou
left a comment
There was a problem hiding this comment.
I will let @me-no-dev and @willmmiles also review.
I am not against the idea of allowing an easy usage of PSRAM for the library especially for objects that are a long lived. This would probably be a good and easy way to free some memory easily for users running with PSRAM.
But I am also wondering why you picked only this place ? For example, middleware could benefit of that too, and a few others little classes also. If we accept the idea, I would probably like to apply this pattern more globally across the project where it can be applied.
| if (!p) { | ||
| p = malloc(size); | ||
| } | ||
| return p; |
There was a problem hiding this comment.
I think that if new fails on ESP32 it does throw std::bad_alloc (which crashes the esp). Here it would return null right ?
There was a problem hiding this comment.
Correct, operator new(size_t) is required to throw std::bad_alloc on an allocation failure. operator new(size_t, const std::nothrow&) is the variant that returns null.
^^^ This. Why just the handler objects? Why not the request objects? The middleware? Response objects? The buffers used by the response objects (usually much bigger than the requests, and probably the largest allocations in the normal path)? The parsed header list entries? ..and so on. I'm not opposed to considering support for custom allocators, but I don't think it makes sense to apply it ad-hoc on only one object type/family. I'd like to see an interface that's scoped over the whole library, with careful selection of which objects and where it's applied and some discussion as to which objects and data structures were selected or not selected, and why. As a second concern, any alternative allocation API should delegate the implementation details to the library user, rather than binding a single specific allocation flavour. Supporting only |
|
Fair points on both counts, thanks! On the operator new semantics: agreed... this should either be the std::nothrow overload pair, or the throwing variant with a proper std::bad_alloc. In our production firmware the null case only occurs when both heaps are exhausted (the device is beyond recovery at that point anyway), but that's no excuse for non-conforming semantics in a library. On scope: I deliberately kept the PR minimal to gauge interest, but I fully support going broader. A pluggable allocator interface (as @willmmiles suggests, similar to what ArduinoJson does) seems like the most future-proof design. I'm happy to rework the PR in that direction if you can settle the preferred shape (static allocator hooks vs. an allocator object), or equally happy if you'd rather drive the design yourselves. |
I like the interface idea, especially because we do not have to deal with the many specific user implémentations. But how to decide when the allocator would be used ? Using PSRAM is slower and psram is best suited for large buffers like images. So one may want to not use psram for requests and responses because he has an app with a lot of short lived clients. also, frequently accessed memory would be in the shared cache with ram: so how much do we gain in putting middleware’s for example, where global ones like security would be accessed on every requests ? So even scoping the psram usage is complicated. in the case of middlewares that is easy because they are created by the user (except if a function is passed). So there is already a way to have the user control how they are allocated. but there could be some situations where this is more fuzzy. For example if someone setups a route for /* as a fallback route, or an error handler , this one probably should be on heap and not psram for speed concerns ? So maybe we could first look at the scopes : what could be and cannot be in psram, and also what kind of allocations the use control. for example, if the user had a way to add handlers to the server in the same way it is done for middleware, then the user could control itself allocation. so maybe we first need also to look if there is a way to refactor the lib to allow the user control a bit more the allocation part per object ? |
|
Let me share some field data on the speed concern first. On the ESP32-S3, PSRAM access goes through the data cache, so small objects that are touched frequently, like a middleware struct or a handler with a callback, are effectively cache resident. After moving all handler objects to PSRAM in our production firmware we could not measure any per request difference, and that firmware has been running since mid July with a dashboard heavy workload and multiple websocket clients. The real limitations of PSRAM are interrupt and DMA access and the short windows during flash writes where the cache is disabled. Neither of those applies to request dispatch in the async_tcp task. For us the motivation was never speed anyway. When internal SRAM runs out, the server does not get slow, it takes down WiFi, mbedTLS and ESP-NOW allocations with it. Compared to keeping the radio alive, a theoretical few microseconds per request is not a trade off in practice. I think your middleware point actually draws the scope line quite nicely. Everything the user constructs themselves, such as middlewares, an AsyncWebSocket instance or custom handlers, is already under user control today. The user can allocate those however they want, so at most this needs documentation. The gap is in the long lived allocations that happen inside the library, where the user has no influence at all: the AsyncCallbackWebHandler created inside server.on(), the AsyncStaticWebHandler from serveStatic(), rewrites and the catch all handler. That is exactly why my PR targeted the handler base class, because that is where all the uncontrollable allocations funnel through. So maybe we can avoid the whole per object policy debate with a phased approach. As a first step, the library gets one global allocator hook, for example two static function pointers or a minimal interface, with the default being the current malloc and new behavior. This hook would only be applied to the long lived objects created by the library itself. The library makes no policy decisions at all. Whoever installs the allocator decides what happens, and by default nothing changes for anyone. Short lived per request objects would stay untouched for now and could be evaluated separately later, since that is where your concern about apps with many short lived clients is entirely valid. I am happy to rework this PR into that first step if the shape works for you, and equally happy to leave the design to you. For our use case, any opt in mechanism that gets the library created long lived objects out of internal SRAM solves the problem. |
Yes I agree that this can be a good starting point. But from an API design perspective it feels wrong to me: it is like providing a patch to the user for something that could be refactored nicely. for example for the on() calls to setup routes: why won’t we allow these methods to take a user constructed handler as parameter directly ? this would also allow the user to have its route in static ram if they want to (flash) and they could even associate the same handler object to several different routes, like: async callback… c = … Such refactoring would let the user have a complete control over allocation but also whether the handers are constructed on heap or static ram on flash. Also, reusing handlers would be possible. I see more advantage of a refactoring of the library than introducing an allocator interface. what do you think guys ? |
|
I like that direction, honestly more than my own proposal. If the user can construct and pass in the handler, allocation control comes for free and no allocator machinery is needed at all. From an API perspective that is the cleaner end state. Two design questions probably need an answer early, because they decide how deep the refactoring goes. The first is ownership. Today the server owns and deletes the handlers it creates internally. Once users pass in their own objects, and especially once the same handler can be attached to several routes as in your example, the server can no longer simply delete what it holds, otherwise we get double deletes. So registration would need to be non owning, or based on shared_ptr, with a clear rule for who destroys what. The second is that the URI currently lives inside the handler itself. AsyncCallbackWebHandler carries its own pattern and method mask, so attaching one handler object to several routes only becomes possible once the route matching moves out of the handler and into the server's routing table. That is the actual refactoring work hiding behind the nice two line example. One small note on flash: handlers are mutable objects, they carry filters, the middleware chain and in the websocket case the client list, so placing them truly in flash would require a const redesign. Static RAM placement on the other hand would just work once construction is in user hands. The convenience overload of on() with a lambda should probably survive in any case, since that is what nearly every existing sketch uses. It would keep allocating internally, which seems fine to me as long as the handler object path exists for those who care about where things live. For what it is worth, we are happy with this as the solution and in no hurry, since we currently carry a small local patch that keeps our firmware running. |
Yes this would be a larger refactoring indeed. |
On ESP32 targets with PSRAM, internal SRAM is often the scarcest resource (lwIP segments, WiFi buffers, WS message queues all live there). Every AsyncWebHandler-derived object (route handlers, catch-all, AsyncWebSocket, static file handlers) currently comes from the internal heap, though handlers are created once after PSRAM init and only read during request dispatch — never from ISR/DMA context, and latency is irrelevant for them.
This PR adds an opt-in build flag -D ASYNCWEBSERVER_USE_PSRAM that routes handler allocation through heap_caps_malloc(MALLOC_CAP_SPIRAM) with a transparent fallback to the internal heap, so it is safe even on targets without PSRAM. Without the flag, behavior is bit-identical to today — no risk for existing users.
Context: we run a dashboard-heavy ESP32-S3 firmware (same field setup that produced #453) where internal SRAM exhaustion cascades into WiFi/ESP-NOW allocation failures. Moving handlers out of internal RAM is one of the mitigations we have been running in production since mid-July.