[3/4] libm/stdlib: Realloc when shrinking by 2* or more

Message ID 20200811230543.2169774-4-keithp@keithp.com
State New
Headers show
Series
  • Fixes for memory allocation bugs
Related show

Commit Message

Eshan dhawan via Newlib Aug. 11, 2020, 11:05 p.m.
This reduces memory usage when reallocating objects much smaller.

Signed-off-by: Keith Packard <keithp@keithp.com>

---
 newlib/libc/stdlib/nano-mallocr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

-- 
2.28.0

Comments

Eshan dhawan via Newlib Aug. 12, 2020, 8:08 a.m. | #1
On Aug 11 16:05, Keith Packard via Newlib wrote:
> This reduces memory usage when reallocating objects much smaller.

> 

> Signed-off-by: Keith Packard <keithp@keithp.com>

> ---

>  newlib/libc/stdlib/nano-mallocr.c | 4 +---

>  1 file changed, 1 insertion(+), 3 deletions(-)

> 

> diff --git a/newlib/libc/stdlib/nano-mallocr.c b/newlib/libc/stdlib/nano-mallocr.c

> index cef23977e..83234c618 100644

> --- a/newlib/libc/stdlib/nano-mallocr.c

> +++ b/newlib/libc/stdlib/nano-mallocr.c

> @@ -476,10 +476,8 @@ void * nano_realloc(RARG void * ptr, malloc_size_t size)

>          return NULL;

>      }

>  

> -    /* TODO: There is chance to shrink the chunk if newly requested

> -     * size is much small */

>      old_size = nano_malloc_usable_size(RCALL ptr);

> -    if (old_size >= size)

> +    if (size <= old_size && (old_size >> 1) < size)

>        return ptr;

>  

>      mem = nano_malloc(RCALL size);

> -- 

> 2.28.0


Ok, so this patch introduces the need for the conditional I complained
about in the previous patch.  IMO, the conditional should be moved
into this patch to introduce it together with the change it requires.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Eshan dhawan via Newlib Aug. 12, 2020, 3:01 p.m. | #2
Corinna Vinschen via Newlib <newlib@sourceware.org> writes:

> Ok, so this patch introduces the need for the conditional I complained

> about in the previous patch.  IMO, the conditional should be moved

> into this patch to introduce it together with the change it requires.


I'm easy; just depends on what you want the history to look like.

-- 
-keith
Eshan dhawan via Newlib Aug. 13, 2020, 7:59 a.m. | #3
On Aug 12 08:01, Keith Packard via Newlib wrote:
> Corinna Vinschen via Newlib <newlib@sourceware.org> writes:

> 

> > Ok, so this patch introduces the need for the conditional I complained

> > about in the previous patch.  IMO, the conditional should be moved

> > into this patch to introduce it together with the change it requires.

> 

> I'm easy; just depends on what you want the history to look like.


Just as I wrote above.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Eshan dhawan via Newlib Aug. 14, 2020, 12:48 a.m. | #4
Corinna Vinschen <vinschen@redhat.com> writes:

> Just as I wrote above.


Done. In reviewing this code further, I've done a bunch more
restructuring to make the code 64-bit clean, removing the 'offset'
mechanism and letting the compiler compute alignment requirements for
us. The diff for that is pretty large though; would there be an interest
in looking at it? I ask because preparing that patch for newlib would
take several hours as it's currently only in picolibc.

https://github.com/keith-packard/picolibc/commit/fd2d18bb5ab442f16789c243648d07b4ec8e2b29

This also uncovers problems when building with newer GCC versions which
"helpfully optimize" common malloc/free operations that cause incorrect
code to be generated in some configurations of the library.

-- 
-keith
Eshan dhawan via Newlib Aug. 17, 2020, 9:54 a.m. | #5
On Aug 13 17:48, Keith Packard via Newlib wrote:
> Corinna Vinschen <vinschen@redhat.com> writes:

> 

> > Just as I wrote above.

> 

> Done. In reviewing this code further, I've done a bunch more

> restructuring to make the code 64-bit clean, removing the 'offset'

> mechanism and letting the compiler compute alignment requirements for

> us. The diff for that is pretty large though; would there be an interest

> in looking at it? I ask because preparing that patch for newlib would

> take several hours as it's currently only in picolibc.

> 

> https://github.com/keith-packard/picolibc/commit/fd2d18bb5ab442f16789c243648d07b4ec8e2b29

> 

> This also uncovers problems when building with newer GCC versions which

> "helpfully optimize" common malloc/free operations that cause incorrect

> code to be generated in some configurations of the library.

> 

> -- 

> -keith


I took a brief look and I'm just wondering why you don't mention
renaming the functions and dropping the wrapper macros in the commit
message.

As for interest, we may want to ask the ARM guys since they provided the
code originally.  Cc'ing Szabolcs as well as Joey, the original author.


Corinna

Patch

diff --git a/newlib/libc/stdlib/nano-mallocr.c b/newlib/libc/stdlib/nano-mallocr.c
index cef23977e..83234c618 100644
--- a/newlib/libc/stdlib/nano-mallocr.c
+++ b/newlib/libc/stdlib/nano-mallocr.c
@@ -476,10 +476,8 @@  void * nano_realloc(RARG void * ptr, malloc_size_t size)
         return NULL;
     }
 
-    /* TODO: There is chance to shrink the chunk if newly requested
-     * size is much small */
     old_size = nano_malloc_usable_size(RCALL ptr);
-    if (old_size >= size)
+    if (size <= old_size && (old_size >> 1) < size)
       return ptr;
 
     mem = nano_malloc(RCALL size);