Re: [PATCH] bench-malloc-thread.c: add assert that malloc() is never out of memory

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 7/3/21 10:28 AM, Gabriel Staples via Libc-alpha wrote:
> ...so we get realistic speed tests. Also add units to the results
> variables. And Replace tabs w/spaces for consistency, since the file
> was mixed format.

Thanks for helping clean up the benchtests.  I don't know if it's 
mentioned in the guidelines, but the compression of 8 spaces into tabs 
is ubiquitous in the glibc sources so I'd suggest keeping them as is. 
This change is not too large and so should be OK without an FSF 
copyright assignment.

> @@ -137,7 +138,14 @@ malloc_benchmark_loop (void **ptr_arr)
>   
>         free (ptr_arr[next_idx]);
>   
> -      ptr_arr[next_idx] = malloc (next_block);
> +      void* ptr = malloc (next_block);
> +      // allocation time isn't realistic if out of memory, so ensure all allocations succeed
> +      if (ptr == NULL)
> +      {
> +          printf ("Out of memory\n");
> +          assert (ptr != NULL);
> +      }
> +      ptr_arr[next_idx] = ptr;

Instead of the assert, please terminate the test with exit 
(EXIT_FAILURE).  Also, please write the error to stderr and not stdout; 
stdout is for json output.

>   
>         iters++;
>       }
> @@ -199,24 +207,24 @@ do_benchmark (size_t num_threads, size_t *iters)
>         *iters = 0;
>   
>         for (size_t i = 0; i < num_threads; i++)
> -	{
> -	  args[i].working_set = working_set[i];
> -	  pthread_create(&threads[i], NULL, benchmark_thread, &args[i]);
> -	}
> +        {
> +          args[i].working_set = working_set[i];
> +          pthread_create(&threads[i], NULL, benchmark_thread, &args[i]);
> +        }
>   
>         for (size_t i = 0; i < num_threads; i++)
> -	{
> -	  pthread_join(threads[i], NULL);
> -	  TIMING_ACCUM (elapsed, args[i].elapsed);
> -	  *iters += args[i].iters;
> -	}
> +        {
> +          pthread_join(threads[i], NULL);
> +          TIMING_ACCUM (elapsed, args[i].elapsed);
> +          *iters += args[i].iters;
> +        }
>       }
>     return elapsed;
>   }
>   
> -static void usage(const char *name)
> +static void usage(const char *executable_name)
>   {
> -  fprintf (stderr, "%s: <num_threads>\n", name);
> +  fprintf (stderr, "Usage: %s: <num_threads>\n", executable_name);
>     exit (1);
>   }
>   
> @@ -239,7 +247,7 @@ main (int argc, char **argv)
>         ret = strtol(argv[1], NULL, 10);
>   
>         if (errno || ret == 0)
> -	usage(argv[0]);
> +        usage(argv[0]);
>   
>         num_threads = ret;
>       }
> @@ -275,14 +283,18 @@ main (int argc, char **argv)
>     d_total_s = cur;
>     d_total_i = iters;
>   
> -  json_attr_double (&json_ctx, "duration", d_total_s);
> +  // Units for x86 processors are "clock cycles"
> +  json_attr_double (&json_ctx, "duration_clock_cycles", d_total_s);
> +  // each iteration is equal to 1 free + 1 malloc
>     json_attr_double (&json_ctx, "iterations", d_total_i);
> -  json_attr_double (&json_ctx, "time_per_iteration", d_total_s / d_total_i);
> -  json_attr_double (&json_ctx, "max_rss", usage.ru_maxrss);
> -
> -  json_attr_double (&json_ctx, "threads", num_threads);
> -  json_attr_double (&json_ctx, "min_size", MIN_ALLOCATION_SIZE);
> -  json_attr_double (&json_ctx, "max_size", MAX_ALLOCATION_SIZE);
> +  // Units for x86 processors are "clock cycles"
> +  json_attr_double (&json_ctx, "time_per_iteration_clock_cycles", d_total_s / d_total_i);
> +  // Maximum resident set size (in kilobytes)
> +  json_attr_double (&json_ctx, "max_rss_kb", usage.ru_maxrss);
> +
> +  json_attr_double (&json_ctx, "num_threads", num_threads);
> +  json_attr_double (&json_ctx, "min_allocation_size_bytes)", MIN_ALLOCATION_SIZE);
> +  json_attr_double (&json_ctx, "max_allocation_size_bytes)", MAX_ALLOCATION_SIZE);
>     json_attr_double (&json_ctx, "random_seed", RAND_SEED);
>   
>     json_attr_object_end (&json_ctx);
> 

Renaming the size is OK to disambiguate, but the time ones are not; 
there may be architectures where the time unit is not clock cycles.

Thanks,
Siddhesh