Re: [PATCH] malloc: Remove bin scanning from memalign (bug 30723)

[DJ Delorie <dj@redhat.com>]

Florian Weimer <fweimer@redhat.com> writes:
> On the test workload (mpv --cache=yes with VP9 video decoding), the
> bin scanning has a very poor success rate (less than 2%).  The tcache
> scanning has about 50% success rate, so keep that.
>
> Update comments in malloc/tst-memalign-2 to indicate the purpose
> of the tests.  Even with the scanning removed, the additional
> merging opportunities since commit 542b1105852568c3ebc712225ae78b
> ("malloc: Enable merging of remainders in memalign (bug 30723)")
> are sufficient to pass the existing large bins test.
>
> Remove leftover variables from _int_free from refactoring in the
> same commit.

LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>

> -  mchunkptr nextchunk;         /* next contiguous chunk */
> -  INTERNAL_SIZE_T nextsize;    /* its size */
> -  int nextinuse;               /* true if nextchunk is used */
> -  INTERNAL_SIZE_T prevsize;    /* size of previous contiguous chunk */
> -  mchunkptr bck;               /* misc temp for linking */
> -  mchunkptr fwd;               /* misc temp for linking */

No longer needed in this scope, ok.

> @@ -5032,42 +5026,6 @@ _int_realloc (mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize,
>     ------------------------------ memalign ------------------------------
>   */
>  
> -/* Returns 0 if the chunk is not and does not contain the requested
> -   aligned sub-chunk, else returns the amount of "waste" from
> -   trimming.  NB is the *chunk* byte size, not the user byte
> -   size.  */
> -static size_t
> -chunk_ok_for_memalign (mchunkptr p, size_t alignment, size_t nb)
> -{
>   . . .
> -}

Only used for bin scanning, so no longer needed.  Ok.

> -  mchunkptr victim;

No longer needed, ok.

> -  /* This will be set if we found a candidate chunk.  */
> -  victim = NULL;
> +  /* Call malloc with worst case padding to hit alignment. */
> +  m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));

Consistent with what the remaining code expects, ok.

> +  if (m == 0)
> +    return 0;           /* propagate failure */

Ok.

> -  /* Fast bins are singly-linked, hard to remove a chunk from the middle
> -     and unlikely to meet our alignment requirements.  We have not done
> -     any experimentation with searching for aligned fastbins.  */
> -  if (av != NULL)
> -    {
>   . . .
> -    }

Ok.

> -  /* Strategy: find a spot within that chunk that meets the alignment
> -     request, and then possibly free the leading and trailing space.
> -     This strategy is incredibly costly and can lead to external
> -     fragmentation if header and footer chunks are unused.  */
> -
> -  if (victim != NULL)
> -    {
> -      p = victim;
> -      m = chunk2mem (p);
> -      set_inuse (p);
> -      if (av != &main_arena)
> -	set_non_main_arena (p);
> -    }
> -  else
> -    {
> -      /* Call malloc with worst case padding to hit alignment. */
> -
> -      m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));
> -
> -      if (m == 0)
> -	return 0;           /* propagate failure */
> -
> -      p = mem2chunk (m);
> -    }

No longer needed, ok.

> +  p = mem2chunk (m);

Ok.

> diff --git a/malloc/tst-memalign-2.c b/malloc/tst-memalign-2.c
> index f229283dbf..ecd6fa249e 100644
> --- a/malloc/tst-memalign-2.c
> +++ b/malloc/tst-memalign-2.c
> @@ -86,7 +86,8 @@ do_test (void)
>        TEST_VERIFY (tcache_allocs[i].ptr1 == tcache_allocs[i].ptr2);
>      }
>  
> -  /* Test for non-head tcache hits.  */
> +  /* Test for non-head tcache hits.  This exercises the memalign
> +     scanning code to find matching allocations.  */
>    for (i = 0; i < array_length (ptr); ++ i)
>      {
>        if (i == 4)
> @@ -113,7 +114,9 @@ do_test (void)
>    free (p);
>    TEST_VERIFY (count > 0);
>  
> -  /* Large bins test.  */
> +  /* Large bins test.  This verifies that the over-allocated parts
> +     that memalign releases for future allocations can be reused by
> +     memalign itself at least in some cases.  */
>  
>    for (i = 0; i < LN; ++ i)
>      {

Ok.