Re: [Patch] [BZ 15884] strcoll: improve performance by removing the cache

[Leonhard Holz <leonhard dot holz at web dot de>]

Dear Siddhesh,

before I start changing the patch some notes / questions:

> > #define LI_DELIMITER " \n\r\t.,?!"
> > #define FILENAMES_DELIMITER "\n\r"
>
> Why \r ?

Defensive programming... the input files are all generated and might be 
regenerated by some else in the future.

> > typedef struct
> > {
> >    char *str;
> >    size_t strlen;
> >    size_t size;
> >    char **words;
> > } word_list;
> >
> > word_list
>
> Pass and return pointers instead of the whole struct.

Is this really an issue? It's only four times the size of a pointer, the 
benchmark is not affected by it and the malloc's will not make the
code nicer.

> > void
> > test_file (const char *filename, const char *delim, const char *locale)
> > {
> >    setlocale (LC_ALL, locale);
>
> Test return value of setlocale.
>
> >    timing_t start, stop, cur;
> >    size_t i, iters = INNER_LOOP_ITERS;
> >
> >    printf ("%-50s %-10s", filename, setlocale (LC_ALL, NULL));
>
> It is sufficient to print the locale value passed to this function
> here once you ensure that the setlocale return value is successful.

The idea is to set a fallback locale at the beginning which is used if 
one of the benchmarked locales are not available. This is why the used 
locale is also printed in the results. So you want it to skip the run if 
the needed locale is not available?

> >    char *text = read_file (filename);
> >    word_list list = tokenize_string (text, delim);
> >
> >    word_list *tests = malloc (INNER_LOOP_ITERS * sizeof (word_list));
> >    for (i = 0; i < INNER_LOOP_ITERS; i++)
> >      tests[i] = copy_word_list (list);
>
> This will obviously have to be adjusted (and hence made cleaner) once
> copy_word_list and all other functions above pass pointers to
> word_list.

Maybe I'll see when I change it but maybe you can give me a hint what 
you mean with "cleaner".

> >    TIMING_NOW (start);
> >    for (i = 0; i < INNER_LOOP_ITERS; i++)
> >      qsort (tests[i].words, tests[i].size, sizeof (char *), compare_words);
> >    TIMING_NOW (stop);
> >
> >    setlocale (LC_ALL, "en_US.UTF-8");
> >
> >    TIMING_DIFF (cur, start, stop);
> >    TIMING_PRINT_MEAN ((double) cur, (double) iters);
> >    putchar ('\n');
> >
> >    for (i = 0; i < INNER_LOOP_ITERS; i++)
> >      free_word_list (tests[i]);
> >    free (tests);
> >
> >    free_word_list (list);
> >    free (text);
> > }
>
> I'd like the printed output to be in JSON format, but we can do the
> first cut in plain text if you're willing to add an extra improvement
> later to make the output in JSON.

Is there some code that can be reused for output in JSON format?

> > int
> > do_bench (void)
> > {
> >    if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
> >      {
> >        printf ("Failed to set default locale.");
> >        return 1;
> >      }
>
> Why do you need this?

See above.

Thank you for your time,
Leonhard