Dynamically allocate the SD sector cache on use, save ~512 bytes when SD not used #4204
Conversation
earlephilhower
force-pushed
the
dynamic_sd_cache
branch
from
19526b9 to
4747e5c
Compare
The SD library includes a 512-byte cache array as a static class member to hold a copy of a sector on the SD card. Because it was statically allocated, the mere act of including <SD.h> in either your own application or *any library* you use--even if you didn't ever use any of the SD objects--would cause the linker to allocate a 512 byte array in BSS out of heap. Replace this static allocation with a static pointer, and only allocate that pointer on the instantiation of a SdVolume object. Free it when the SdVolume is destroyed.
earlephilhower
force-pushed
the
dynamic_sd_cache
branch
from
4747e5c to
35dd992
Compare
libraries/SD/src/utility/SdFat.h
Outdated
| @@ -431,14 +431,16 @@ union cache_t { | |||
| class SdVolume { | |||
| public: | |||
| /** Create an instance of SdVolume */ | |||
| SdVolume(void) :allocSearchStart_(2), fatType_(0) {} | |||
| SdVolume(void) :allocSearchStart_(2), fatType_(0) { if (!cacheBuffer_) cacheBuffer_ = (cache_t *)malloc(sizeof(cache_t)); } | |||
There was a problem hiding this comment.
null is not checked. I don't really know myself what to do in constructors when this happen.
Maybe you can also help with this in #4134 :)
There was a problem hiding this comment.
I believe errors in constructors can only throw an exception. Unfortunately, that's not the Arduino way, so I'm as at a loss as yourself.
The alternative here, and elsewhere, is to use lazy allocation and/or defensive. Before use of the cache it can be checked and the method can error out if it can't be allocated, assuming the API allows it. Perusing the SD headers, it looks like SEGV'ing is really the only thing that's possible using the current API.
Something like link time optimization (-flto) might eliminate the need for this kind of change as it should be able to detect no call graph into this static class variable, but I've not been able to make it work (and I believe we're upgrading GCC soon, and at that point it should be "automatic" anyway).
platform.txt
Outdated
| @@ -86,7 +86,7 @@ recipe.S.o.pattern="{compiler.path}{compiler.c.cmd}" {compiler.cpreprocessor.fla | |||
| recipe.ar.pattern="{compiler.path}{compiler.ar.cmd}" {compiler.ar.flags} {compiler.ar.extra_flags} "{build.path}/arduino.ar" "{object_file}" | |||
|
|
|||
| ## Combine gc-sections, archives, and objects | |||
| recipe.c.combine.pattern="{compiler.path}{compiler.c.elf.cmd}" {compiler.c.elf.flags} {compiler.c.elf.extra_flags} -o "{build.path}/{build.project_name}.elf" -Wl,--start-group {object_files} "{build.path}/arduino.ar" {compiler.c.elf.libs} -Wl,--end-group "-L{build.path}" | |||
| recipe.c.combine.pattern="{compiler.path}{compiler.c.elf.cmd}" -Wl,-M -Wl,-Map "-Wl,{build.path}/{build.project_name}.map" {compiler.c.elf.flags} {compiler.c.elf.extra_flags} -o "{build.path}/{build.project_name}.elf" -Wl,--start-group {object_files} "{build.path}/arduino.ar" {compiler.c.elf.libs} -Wl,--end-group "-L{build.path}" | |||
|
@d-a-v That's what I get for doing two things at once. Just removed that change, sorry! |
libraries/SD/src/utility/SdFat.h
Outdated
| SdVolume(void) :allocSearchStart_(2), fatType_(0) {} | ||
| SdVolume(void) :allocSearchStart_(2), fatType_(0) { if (!cacheBuffer_) cacheBuffer_ = (cache_t *)malloc(sizeof(cache_t)); } | ||
| /** Delete an instance of SdVolume */ | ||
| ~SdVolume() { free(cacheBuffer_); cacheBuffer_ = NULL; } |
There was a problem hiding this comment.
The original cache was a static array, accessible from any instance. This change makes it a static pointer, and the constructor checks if it has been allocated before, in which case nothing is done, or not, in which case it is allocated.
This destructor just blindly frees the mem. If there is more than one instance, the others will then try to access after free.
I think it's unlikely that there will be more than one instance of this class, but I think this is something that should be fixed to cover e.g.: temps (pass by value) and copy.
I suggest use of a shared_ptr.
There was a problem hiding this comment.
I had only considered the single global SD object. Since then, with the BSSL stuff I've been introduced to the wonders of the std::shared_ptr and this would be a good use of one like you suggest. Will see if I can fix it up this weekend.
There was a problem hiding this comment.
Done, but using a separate refcnt instead of std::ptr as there needs to be a special-case for cnt==1 (like the BearSSL issue).
The cache now is only freed when there are no references to it (i.e. if for some reason multile SD objects were used). Files in SD/src/utility converted to UNIX format (matching other libs)
earlephilhower
force-pushed
the
dynamic_sd_cache
branch
from
4053bc1 to
b998a5d
Compare
earlephilhower
force-pushed
the
dynamic_sd_cache
branch
from
b998a5d to
8dcbf46
Compare
|
There's been some large changes to SD since this was submitted, so drop it for the near term. |
Related to issue #3740
The SD library includes a 512-byte cache array as a static class member
to hold a copy of a sector on the SD card. Because it is statically
allocated, the mere act of including <SD.h> in either your own application
or any library you use--even if you didn't ever use any of the SD
objects--would cause the linker to allocate a 512 byte array in BSS out
of heap.
Replace this static allocation with a static pointer, and only allocate
that pointer on the instantiation of a SdVolume object. Free it when
the SdVolume is destroyed.
The simple test example below shows the issue:
cat > sdcardtest.ino <<EOF
#include <SD.h>
void setup() {
Serial.begin(115200);
Serial.println("Hello, world!");
}
void loop() {
}
EOF
Examining the memory map shows the large buffer that's allocated w/o anyone
ever actually allocating a SD object.
earle@/tmp$ readelf -a /tmp/arduino_build_705221/*.elf | grep cacheBuff
614: 3ffee9b8 512 OBJECT GLOBAL DEFAULT 3 _ZN8SdVolume12cacheBuffer