Move timezone table to PROGMEM #3766
Conversation
blazoncek
left a comment
There was a problem hiding this comment.
This works like charm and does what it says! 😄
The two strings I commented do not need flash storage as they are used elsewhere. I'd like them reverted though.
wled00/set.cpp
Outdated
| bool handleSet(AsyncWebServerRequest *request, const String& req, bool apply) | ||
| { | ||
| if (!(req.indexOf("win") >= 0)) return false; | ||
| if (!(req.indexOf(F("win")) >= 0)) return false; |
There was a problem hiding this comment.
This one is unnecessary as "win" is also used elsewhere and AFAIK same strings only occupy space once if not stored in PROGMEM.
There was a problem hiding this comment.
It's true that string literals can be deduplicated by the linker, while PROGMEM arrays have to be deduplicated by the programmer. Unfortunately, our ESP8266 RAM is basically filled with them. I'll see if I can get some metrics, but we're looking at many kb in total. Browsing the RAM layout, it's just page after page after page of static strings, almost all of which could be in PROGMEM if the surrounding code supported it.
I've got a note in my todo to look at this from a more systematic perspective - I want to prod C++11 user-defined literals and the linker scripts - but I don't think I'll get to that for a while. For the forseeable future, about the only option we have is to go through literal by literal and move everything we can, one patch at a time.
In any event, I'll drop 4c49241 from the PR, and we can consider that process elsewhere.
wled00/set.cpp
Outdated
| if (subObj[name].isNull()) { | ||
| // the first occurrence of the field describes the parameter type (used in next loop) | ||
| if (value == "false") subObj[name] = false; // checkboxes may have only one field | ||
| if (value == F("false")) subObj[name] = false; // checkboxes may have only one field |
|
Just to complement the picture, here are the "savings" on esp32
For this PR, we only have a small increase of both RAM and FLASH size, but saving nothing on esp32. Current stats for [env:esp32dev_V4_dio80] buildenv: My conclusion is that - in contrast to this statement - "progmem-fu" exercises are sometimes harmful on esp32. It looks like while trying to optimize for 8266, we are filling up flash on esp32, without any added value for esp32 users? |
Yes. That's the point, ESP8266 needs every byte of RAM possible. |
Wow, I didn't check these metrics - I'm very surprised! I'll review the compiler output and see why that's the case. A fixed table shouldn't be any larger than inline constructors. Possibly there's alignment issues, I'll report back with what I find. |
f778655 to
108978d
Compare
Yes this was my first thought, too. it could also be related to differences in compiler/linker optimizations - esp32 uses function-based unused code removal ( |
|
It turend out to be simpler than that - I just forgot to Thank you guys for the thorough review. |
|
@softhack007 if you agree, please merge. I'll then cherry-pick into main branch. |
I agree - merged :-) |
The switch-case code architecture was placing the timezone specifications in RAM. Move them to PROGMEM, saving ~200 bytes.
Also includes a small patch to move a couple settings strings to PROGMEM, too.