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

better / more efficient realloc() fix from upstream umm #4047

Closed
d-a-v opened this issue Dec 30, 2017 · 6 comments
Closed

better / more efficient realloc() fix from upstream umm #4047

d-a-v opened this issue Dec 30, 2017 · 6 comments
Assignees
Milestone

Comments

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 30, 2017

author's fixing branch with test: link

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jan 23, 2018

following discussion from #4134

Any particular case you're trying to fix/optimize, that can be tested against his changes? W/o a compelling reason, IMO it's a bit premature to pull in such fresh code in a bit of the core that's so important.

@earlephilhower You are right. Extensive tests must be done, and testing malloc (only realloc here) is not an easy task especially if there's improvement to see. The author included separate unit-test, we have umm internal integrity check, and one could setup a multi-strings manipulation sketch that would fill ram with some kind of success score to compare with the new proposed realloc.

@earlephilhower
Copy link
Collaborator

@d-a-v Actually, if the assumption is that "git head (from date XXXX to date YYYY) is unstable and may contain code that breaks your app, and should only be used to verify operation" maybe it's not so bad after all. We do some focused tests, somehow (maybe an app that's prone to breaking doesn't break for a series of tests), and then merge and let the larger community test.

It's not the friendliest of things, but "git head" is often unstable, no?

@d-a-v
Copy link
Collaborator Author

d-a-v commented Jan 24, 2018

I believe head may also have experimental patch but I guess we are now on a freezing process for 2.4.1 fixing-release.
Anyway about UMM, I am going to use a malloc-hole-metric to check whether the new upstream realloc is better. Realloc is a lot used at least with String which are extensively used in sketchs, so it's worth the test.

@rhempel
Copy link

rhempel commented Feb 4, 2018

Thanks for using umm_malloc()! My test suite passed before the realloc() defect was reported - so obviously it was not a good enough suite. Memory manager bugs are notoriously hard to find, and I appreciate that your team is sticking with umm_malloc().

Please keep me up to date on progress, and once the (issue-11) branch is verified, I will merge back to (master) and bump the release version so that everyone gets the latest and greatest.

@earlephilhower earlephilhower added this to the 2.6.0 milestone Jan 16, 2019
@earlephilhower
Copy link
Collaborator

Seems like a good thing to pull in. Seems to have just gotten lost in the 2.4.1 crunch.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Aug 29, 2019

#6438 will handle this

@d-a-v d-a-v closed this as completed Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants