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

umm_malloc manual merge with upstream #7337

Merged
merged 12 commits into from
Jun 8, 2020

Conversation

mhightower83
Copy link
Contributor

@mhightower83 mhightower83 commented May 29, 2020

Noteworthy Changes:

  • ESP.getHeapFragmentation() is now calculated with blocks. Previously the calculation was done with bytes.
  • If frequent calls are made to ESP.getHeapFragmentation(), -DUMM_INLINE_METRICS would reduce long periods of interrupts disabled caused by frequent calls to umm_info(). Instead, the computations get distributed across each malloc, realloc, and free. This appears to require an additional 116 bytes of IRAM vs using UMM_STATS with UMM_INFO.
  • When both UMM_STATS and UMM_INLINE_METRICS are defined, macros and structures have been optimized to reduce duplication.
  • ESP.getFreeHeap(), umm_free_heap_size(), xPortGetFreeHeapSize(), system_get_free_heap_size(), ... previously reported values that were 8 bytes too large. In contrast ESP.getMaxFreeBlockSize() was not affected.
  • Fixes the divide by zero exception that was caused by calling ESP.getHeapStats(NULL, NULL, &hfrag); with a fully allocated heap.

New functions from upstream:

  • int umm_usage_metric( void ), returns the ration of 100 * (used/free blocks). Returns -1 when free blocks is zero.
  • int umm_fragmentation_metric( void ), returns the same information as ESP.getHeapFragmentation(). When UMM_INLINE_METRICS is defined ESP.getHeapFragmentation() is a thin wrapper to umm_fragmentation_metric().

* Edited to reflect current PR content.

@devyte devyte added this to the 2.7.2 milestone May 29, 2020
@mhightower83
Copy link
Contributor Author

  • ESP.getMaxFreeBlockSize() is now adjusted to allow for umm_heap overhead. It is now possible for a malloc(ESP.getMaxFreeBlockSize());

@devyte While working through memory boundary calculations on HeapMeteric.ino, I find myself questioning whether I should have done this. At the moment I am on the fence.

@devyte
Copy link
Collaborator

devyte commented May 29, 2020

I think not. The current behavior is that it's not possible to do malloc(maxBlock), which can be unexpected. However, from your explanations on gitter, even if you manage to make it work for maxBlock, it could potentially fail e. g. for maxBlock-overhead, because then the remaining space can't be divided. And that is also unexpected, which means the original cause for fixing it isn't resolved.
Like you said, too many corner cases, and hence too fragile.
I suggest reverting that. A comment has already been added to the docs stating that the maxBlock means free mem chunk, and that size can't be malloced due to overhead.

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.

After re-reading this a couple times it LGTM. The only non-debug path change I caught was the realloc that exactly fits (uncommon, so hard to test but on reading the code looks good). The debug/freeblock stuff I'm less worried about even though the changes are larger since it's not anywhere near as critical as the main malloc code.

@devyte, @d-a-v, can we please get your eyes on this?

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Nothing jumps out at me

@mhightower83
Copy link
Contributor Author

In umm_realloc:

  • The #if defined(UMM_REALLOC_MINIMIZE_COPY) section tracks the content of the upstream with some local macros added. Back when I made my 1st update to umm_malloc PR, I found the upstream had been refactored and removed the defragmenting properties that were originally present. It took some looking to see the logic, it didn't have any comments to make it stand out.
  • I added the #elif defined(UMM_REALLOC_DEFRAG) to recreate and preserve the defragmenting functionality that was lost. This is the default build option we have set in umm_malloc_cfg.h. I have not done any structured testing to confirm; however, I think this to be the best option when considering the amount of reallocates that can occur with the Strings library.

The upstream umm_malloc is a .c program and often does pointer math with void * pointers. When the .c is changed to .cpp the C++ compiler does not accept these. I previously changed most of them to char * in my previous sync up with the upstream. This time round unless a char * value was useful later, I leaned toward using uintptr_t casting.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

I like UMM_INLINE_METRICS even it is still not enabled by default, maybe some more perf tests need to be performed.
Thanks for this fine work, thank you for keeping it sync with upstream.

@earlephilhower earlephilhower merged commit 83523c0 into esp8266:master Jun 8, 2020
@mhightower83 mhightower83 deleted the pr-umm_malloc-refresh branch June 8, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants