Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid large reallocations in webserver's header response #8873

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Feb 28, 2023

No description provided.

@earlephilhower
Copy link
Collaborator

CI seems to be aborting (not code related) frequently right about now. Let's try to re-run later...

@d-a-v d-a-v added the alpha included in alpha release label Feb 28, 2023
@@ -420,7 +420,8 @@ void ESP8266WebServerTemplate<ServerType>::stop() {
}

template <typename ServerType>
void ESP8266WebServerTemplate<ServerType>::sendHeader(String&& name, String&& value, bool first) {
template <typename S1, typename S2>
void ESP8266WebServerTemplate<ServerType>::sendHeader(S1 name, S2 value, bool first) {
Copy link
Collaborator

@mcspr mcspr Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit

at a glance, we have a bunch of typed pairs that generate multiple emplace_... for every sendHeader variant
I'd consider using perfect forward and a specific type we already have

using Pair = std::pair<String, String>;

auto foo(Pair pair, bool condition) {
    if (condition) {
        foo(std::move(pair));
    } else {
        bar(std::move(pair));
    }
}

template <typename S1, typename S2>
auto foo(S1&& name, S2&& value, bool condition) {
    foo(std::make_pair(
        String(std::forward<S1>(name)),
        String(std::forward<S2>(value)), condition);
}

@hmueller01
Copy link
Contributor

@d-a-v I have pulled your branch and did a quick test. Looks good. Thanks for the quick fix!

What I found when more that one client is connecting to the WebServer is this:

[String] 'User-Agent ... ome/110.0.': Reallocating large String(143 -> 144 bytes)
[String] 'User-Agent ... 53 Mobile ': Reallocating large String(159 -> 160 bytes)

It's surely not releated to this change and I even do not know if it comes from ESPWebServer or our code ...

for (const auto& kv: _userHeaders)
if (!_streamHeader(kv.first, kv.second))
return false;
_userHeaders.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will these be cleared if streaming failed?

N.B. I got the impression the current implementation of the webserver does hold some data way longer than needed as opening a web page may sometimes free up quite a bit of memory. (maybe POST data of a previous request???)

@hmueller01
Copy link
Contributor

I think the same happens in the ESP8266HTTPClient.cpp#addHeader ...

        if (first) {
            _headers = headerLine + _headers;
        } else {
            _headers += headerLine;
        }

No one knows in advance how big _headers grows. Resulting in:

[String] 'x-ESP8266- ...  1478656
': Reallocating large String(134 -> 165 bytes)
[String] 'x-ESP8266- ... : 548896
': Reallocating large String(165 -> 221 bytes)
[String] 'x-ESP8266- ... afd6bfc5
': Reallocating large String(221 -> 251 bytes)
[String] 'x-ESP8266- ...  2097152
': Reallocating large String(251 -> 290 bytes)
[String] 'x-ESP8266- ... b29dcd3)
': Reallocating large String(290 -> 315 bytes)
[String] 'x-ESP8266- ...  version
': Reallocating large String(315 -> 338 bytes)
[String] 'x-ESP8266- ... sion: 41
': Reallocating large String(338 -> 357 bytes)

@hmueller01
Copy link
Contributor

@d-a-v Is there any progress on this PR / issue #8872?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpha included in alpha release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants