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

OOM debug: warn about String reallocation #7908

Merged
merged 5 commits into from
Mar 14, 2021
Merged

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Mar 4, 2021

When OOM debug is enabled: User helper to warn about misused Strings

With this sketch:

void setup ()
{
    Serial.begin(115200);
    String s;

    Serial.printf("empty\n");
    for (int i = 0; i < 170; i++)
    {
        char c = 'a' + (i % 26);
        s += c;
        Serial.printf("+%c (%2u) %s\n", c, s.length(), s.c_str());
    }
}

void loop ()
{
    delay(1000);
}
[... no warning from 1 to 140]
22:26:24.074731667 +j (140) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghij
22:26:24.076687969 +k (141) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijk
22:26:24.095628380 +l (142) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijkl
22:26:24.097935792 +m (143) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklm
22:26:24.105040649 [offending String op 143->144 ('abcdefghij ... defghijklm')]
22:26:24.123835556 +n (144) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmn
22:26:24.144697970 +o (145) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmno
22:26:24.150967332 +p (146) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnop
22:26:24.167547088 +q (147) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopq
22:26:24.188924455 +r (148) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqr
22:26:24.196662949 +s (149) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrs
22:26:24.211387159 +t (150) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrst
22:26:24.213691185 +u (151) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstu
22:26:24.232972928 +v (152) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuv
22:26:24.254602860 +w (153) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvw
22:26:24.256531706 +x (154) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwx
22:26:24.277578340 +y (155) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxy
22:26:24.299916179 +z (156) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
22:26:24.302642315 +a (157) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyza
22:26:24.321516961 +b (158) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzab
22:26:24.332390725 +c (159) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabc
22:26:24.334352380 [offending String op 159->160 ('abcdefghij ... tuvwxyzabc')]
22:26:24.351496870 +d (160) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcd
22:26:24.373622505 +e (161) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcde
22:26:24.375705812 +f (162) abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

I'm not sure it makes sense to print on every string reallocation. They are expected and normal, no? Seems like in a real app this might spew a lot of messages...

@d-a-v
Copy link
Collaborator Author

d-a-v commented Mar 4, 2021

Frequent String reallocation is the ram-without-mmu fatal disease, users should fix their code or disable OOM debug.
If that's too hurting, what about printing a message at least once per String ?

(edit1)

They are expected and normal, no?

I'd say they are possible, silent in non-OOM debug mode, abnormal in this environment, and avoidable with String::reserve().

(edit2)
I'd add that if user is too much bugging the core, the core is in its damn right to bug the user back ;)

(edit3)
It is not clear in the log above, that the first reallocation is silent (mention added)

@earlephilhower
Copy link
Collaborator

Frequent String reallocation is the ram-without-mmu fatal disease, users should fix their code or disable OOM debug.
If that's too hurting, what about printing a message at least once per String ?

Okay, I see your point and it is true that String (re)allocations can be ugly with many people's code. Maybe only spew if size is large enough to be an issue? Say, >128bytes? Reallocating from 16b to 32b isn't really a big deal, but reallocating from 768b to (768+delt) bytes is since you're going to need a massive free zone to find the space for it.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Mar 5, 2021

128bytes it is, configurable in the added define

@d-a-v d-a-v added the alpha included in alpha release label Mar 5, 2021
@earlephilhower earlephilhower added this to the 3.0.0 milestone Mar 13, 2021
@d-a-v d-a-v merged commit 4440021 into esp8266:master Mar 14, 2021
@d-a-v d-a-v deleted the stroom branch March 14, 2021 06:57
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.

2 participants