-
Notifications
You must be signed in to change notification settings - Fork 13.3k
metric for heap fragmentation #5090
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think this is a must-have that should always be available to the user. It adds to a "health" metric of the esp, along with free heap, and the stack metrics.
I'd like it more without global state, though. Maybe a new function specific to the calculation or something?
cores/esp8266/Esp-unfragness.cpp
Outdated
#include "coredecls.h" | ||
#include "Esp.h" | ||
|
||
uint16_t EspClass::getHeapUnfragness(uint32_t* freeHeap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I personally love the name, and the term "unfragmentation-ness", maybe it would be better for users to rename to something like contigousness (or an abbreviation like contig), or calculate the complement and call it fragmentation, which is a well-known term.
cores/esp8266/Esp-unfragness.cpp
Outdated
|
||
uint16_t EspClass::getHeapUnfragness(uint32_t* freeHeap) | ||
{ | ||
ummFreeSize2 = 1; // enable processing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like global state. Shouldn't these lines be protected against interrupts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to do this without global state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this code be explained with comments or a url or doc reference?
cores/esp8266/Esp-unfragness.cpp
Outdated
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA | ||
*/ | ||
|
||
#ifdef DEBUG_ESP_PORT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good metric to have available as a tool, just like free heap. I suggest not making it debug-only.
@@ -1025,6 +1029,11 @@ void ICACHE_FLASH_ATTR *umm_info( void *ptr, int force ) { | |||
++ummHeapInfo.freeEntries; | |||
ummHeapInfo.freeBlocks += curBlocks; | |||
|
|||
#ifdef DEBUG_ESP_PORT | |||
if (ummFreeSize2) | |||
ummFreeSize2 += (uint64_t)curBlocks * (uint64_t)sizeof(umm_block) * (uint64_t)curBlocks * (uint64_t)sizeof(umm_block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary for this to be a uint64_t? I think for the esp it's not necessary to use 64bit arithmetic, but as a general extension to umm it may be good. I can't tell what should be done
I have no idea. I reckon
It is global but only used during the calculation. It's the way umm works: it uses a global structure (
It's the L2 / Euclidian norm of free block sizes. We have
As it is and if it is always enabled, it takes ~160 bytes in flash when not called, and 1KB when called (I guess
|
Now always enabled. Adds 16 bytes in flash, and another 128 bytes when used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still traveling so haven't looked in depth yet, but this seems like a great idea!
If |
|
cores/esp8266/Esp.cpp
Outdated
@@ -171,6 +171,11 @@ uint32_t EspClass::getFreeHeap(void) | |||
return system_get_free_heap_size(); | |||
} | |||
|
|||
uint16_t EspClass::getMaxFreeBlockSize(void) | |||
{ | |||
return system_get_free_heap_size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this and the return via maxBlock pointer in getHeapFragmentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mistake :) (the example uses getHeapFragmentation parameters)
cores/esp8266/Esp-frag.cpp
Outdated
#include "coredecls.h" | ||
#include "Esp.h" | ||
|
||
uint8_t EspClass::getHeapFragmentation (uint32_t* freeHeap, uint16_t* maxBlock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it bothers me that we have a getFreeHeap() that takes no args and return only the free heap, a getMaxBlockSize() that takes no args and returns only the max block size, and this that does take 2 optional args for free heap and maxblock and returns the fragmentation. It just seems inconsistent to me. I think I'd prefer to have 3 standalone functions one for each, plus a 4th function getHeapStats() or whatever that takes 3 args for getting everything at once (performance) and returns nothing (void).
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the three functions (Free, MaxBlock, Fragmentation), the same umm_info()
function is called which goes through every heap chunks. I figured that those who would call Fragmentation may optionally give pointers to fill those results for free (as this one may not be quite often used).
OK with getHeapStats()
.
Awesome test sequence. I was thinking it'd be great adding that as a device test, but then we'd have to update the test every time we implement something that frees more heap, because the numbers could change. Not sure if that's desirable. Maybe just add it as an example, and include the expected output at the time of implementation? |
I will add the example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some remaining questions at this point.
cores/esp8266/Esp-frag.cpp
Outdated
#include "coredecls.h" | ||
#include "Esp.h" | ||
|
||
void EspClass::getHeapStats(uint32_t* free, uint16_t* max, uint8_t* frag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arg "free" shadows free(), arg max can shadow max() (many apps #define it). would be better to change the names.
@@ -1761,4 +1762,13 @@ size_t ICACHE_FLASH_ATTR umm_free_heap_size( void ) { | |||
return (size_t)ummHeapInfo.freeBlocks * sizeof(umm_block); | |||
} | |||
|
|||
uint16_t ICACHE_FLASH_ATTR umm_max_block_size( void ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the umm code doesn't seem to rely on these uint8_t/uint16_t/uint32_t types. Maybe it would be better to stick to the types used in the rest of the code, i.e.: go low level adventurer and declare them as unsigned char/unsigned short/unsigned int
@@ -1024,6 +1024,7 @@ void ICACHE_FLASH_ATTR *umm_info( void *ptr, int force ) { | |||
if( UMM_NBLOCK(blockNo) & UMM_FREELIST_MASK ) { | |||
++ummHeapInfo.freeEntries; | |||
ummHeapInfo.freeBlocks += curBlocks; | |||
ummHeapInfo.ummFreeSize2 += (uint32_t)curBlocks * (uint32_t)sizeof(umm_block) * (uint32_t)curBlocks * (uint32_t)sizeof(umm_block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe don't rely on these types and use basic types like in the rest of umm
cores/esp8266/Esp-frag.cpp
Outdated
if (max) | ||
*max = ummHeapInfo.maxFreeContiguousBlocks * block_size; | ||
if (frag) | ||
*frag = 100 - (sqrt32(ummHeapInfo.ummFreeSize2) * 100) / fh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something like this be more precise, or same output? I ask because paranoid about numerical stability.
100 - (sqrt32( ummHeapInfo.ummFreeSize2 * 10000) / (fh*fh) );
There was a problem hiding this 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'd be more numerically stable, but it would takes more flash (16 bytes), more ram (4 more bytes) and more times (64 instead of 32 bits ints).
About range, the current version works with at most 64K free heap.
@@ -171,6 +172,11 @@ uint32_t EspClass::getFreeHeap(void) | |||
return system_get_free_heap_size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that this sdk api is the same as:
uint32_t fh = ummHeapInfo.freeBlocks * block_size;
...
*free = fh;
used in getHeapStats()? After a quick look I can't tell how the integration works between that sdk api and umm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this is a fw call that calls fw api'sxPortGetFreeHeapSize
in which we call umm (heap.c).
This is a great addition. And just curious, what would be a good default allocation size for Strings to minimize fragmentation? |
@TD-er I did not measure time.
Only testing would answer this tricky question because it depends on everything (how are these strings used). |
V2.5.0 gives me this error now:
Did you guys just removed the |
Available only in debug mode,to help debugging.ESP.getHeapUnfragness()
calculates "unfragmentation-ness" of the heap.Whatever free heap value, it gives 1000 when the whole free memory is contiguous and decreases with fragmentation.
For example it gives 707 (which is 1000/sqrt(2)) for the same free size when it is in fact two equal-size continuous blocks. 999 is given with the same free size with a huge free block and a small one.
Here is a sketch log when allocating many blocks, then removing half of them (every other), then removing again half of them, with free heap and fragmentation metric:
(plenty of available space, but heap is in very bad shape: 150/1000)