Cleanup base64::encode functions #6607
Conversation
dirkmueller
force-pushed
the
base64_opt
branch
from
8c10574 to
8ef7244
Compare
|
I'll play with NRVO a bit, but need to rebuild my test setup first before pushing the change. |
dirkmueller
changed the title
dirkmueller
force-pushed
the
base64_opt
branch
from
8ef7244 to
ddee26c
Compare
|
Could we just use the ROM base64 functions with a tiny wrapper, instead? |
|
I've seen that as well, but I couldn't find a header definition with the calling convention or any reference to it in the espressif nonos sdk documentation. Any idea where to find this? And can we remove libb64/ ? Otherwise this won't remove code size.. there was conflicting information about whether or not it is part of the api guarantees. |
|
Looking at the disassembly, I think it's safe to ignore my suggestion, @dirkmueller . I see it does some math and uses the ROM malloc implementation, which is definitely a no-no for our apps. |
dirkmueller
force-pushed
the
base64_opt
branch
2 times, most recently
from
d6737b5 to
e7e5577
Compare
|
@devyte please reevaluate the 3.0 tagging. I think this can be merged any time now. |
cores/esp8266/base64.cpp
Outdated
| String base64 = String(buffer); | ||
| free(buffer); | ||
| return base64; | ||
| int len = base64_encode_block((const char *) &data[0], length, (char*)base64.begin(), &_state); |
There was a problem hiding this comment.
Instead of this, can we have base64_encode_block take a String instead and use the String += <char> way of setting it. That keeps base64 out of String's innards, while only requiring the single allocation of the block w/String::reserve.
There was a problem hiding this comment.
well, thats the question. in the discussion earlier it was said that libb64/ is part of the API and can't be changed, so I don't want to create yet another wrapper around it.
if we stop considering libb64/ being part of the API then I'd be happy to fold this into base64.cpp and clean it up and make it use String() directly.
dirkmueller
force-pushed
the
base64_opt
branch
2 times, most recently
from
340af13 to
da2ebaf
Compare
The implementation choice here using libb64 is generally good as it is a relatively fast implementation, however the adaptation to use PROGMEM for the translation function was a bad choice, as reading randomly PROGMEM with byte-wide access is very very very slow. Doing a naive if-snake is between 20% and 55% faster and uses less flash (about 120 bytes less) and also for reasons I don't understand 8 bytes less data RAM (maybe the removal of static?). In addition the base64::encode function was allocating for larger input a huge amount of memory (twice the total size). we can reduce that by doing a chunk-wise conversation to base64.
Rather than first creating a string with newlines and then stripping it away in the fast path of constructing the query, we can call the right method and trust that the result does not have newlines anymore.
dirkmueller
force-pushed
the
base64_opt
branch
from
da2ebaf to
da26de2
Compare
|
@devyte please also reconsider the "target 3.0.0" labeling here, there is no API change anymore, so this should be suitable for any release. |
|
Done. |
dirkmueller
changed the title
There was a problem hiding this comment.
LGTM with the note to @devyte. At worst, this isn't any worse than what was there, but at best it could be a great improvement.
The implementation choice here using libb64 is generally good as it
is a relatively fast implementation, however the adaptation to
use PROGMEM for the translation function was a bad choice, as reading
randomly PROGMEM with byte-wide access is very very very slow.
Doing a naive if-snake is between 10% and 50% faster and uses less
flash (about 120 bytes less) and also for reasons I don't understand
8 bytes less data RAM (maybe the removal of static?).