Created attachment 127248 [details] [review]
Optimization to avoid loading all .pc files

Created attachment 127255 [details] [review]
Optimization to avoid loading all .pc files

Comment on attachment 127255 [details] [review]
Optimization to avoid loading all .pc files

Review of attachment 127255 [details] [review]:
-----------------------------------------------------------------

I think this change makes sense, but there's a bit of cleanup for the patch. Have you done any performance testing to see if this really is an optimization? I imagine that you have to have a lot of pc files before this makes a difference since you trade off scanning all files once vs repeatedly walking the search path.

It would be great if you could look at a couple workloads to see what the effects are. You can simply time some measurements in different cases. Another option is to call pkg-config under perf and see where the time is spent. A third is to build pkg-config with profiling support and run it under gprof. See https://sourceware.org/binutils/docs/gprof/ for details on that, it's pretty straightforward.

::: pkg.c
@@ +118,5 @@
>      return FALSE;
>  }
>  
> +static Package *
> +internal_get_package (const char *name, gboolean warn);

Please leave a newline between this declaration and the next line.

@@ +162,3 @@
>    if (!dir)
>      {
> +      debug_spew ("Cannot open directory '%s' in package search path: %s\n", dirname, g_strerror (errno));

Please wrap long lines at roughly 72 columns.

@@ +215,3 @@
>        g_list_foreach (search_dirs, (GFunc)scan_dir, NULL);
> +      else
> +        add_virtual_pkgconfig_package ();

Please indent this section correctly and add a comment about why add_virtual_pkgconfig_package is being skipped in the want_list case.

@@ +238,4 @@
>    if ( ends_in_dotpc (name) )
>      {
>        debug_spew ("Considering '%s' to be a filename rather than a package name\n", name);
> +      location = (char*)name;

I think you would want to strdup name in this case so you can free location at the end.

@@ +261,4 @@
>              }
>          }
>        
> +      GList *dir_iter;

Please declare variables at the top of the block. I know this is handled fine, but the current convention is not to do inline declarations.

@@ +267,5 @@
> +        {
> +          path_position++;
> +          location = g_strdup_printf ("%s%c%s.pc", (char*)dir_iter->data, G_DIR_SEPARATOR, name);
> +          if (g_file_test (location, G_FILE_TEST_IS_REGULAR))
> +            break;

This leaks the allocation since location is never freed in the successful case. Like the above `location = name` case, I think you need to free location below.

@@ +295,4 @@
>        const char *end = name + (len - EXT_LEN);
>        const char *start = end;
>  
> +      while (start != name && start[-1] != G_DIR_SEPARATOR)

What's this change about?

@@ +1161,5 @@
>    ignore_requires = TRUE;
>    ignore_requires_private = TRUE;
>  
> +  g_hash_table_foreach (packages, max_len_foreach, &mlen);
> +  g_hash_table_foreach (packages, packages_foreach, GINT_TO_POINTER (mlen + 1));

Now that we're looking directly at the packages hash table, can you change packages_foreach so that it simply uses the key that was passed to it? In other words, there's no reason to call get_package again because we're already looking at the valid package hash table. In that case, `Package *pkg = value` in packages_foreach would take care of it.

Created attachment 128206 [details] [review]
Optimization to load only needed .pc files.

Hi Dan!

Attached is a new version of the patch to address the issues you raised. It was created with "git format-patch -w -1", but I can do it with "git format-patch -1" if it improves readability/applicability.

Please confirm the issues have been addressed. I'll do performance testing as soon as I know it is on a good enough state to be committed.

Thanks!

Created attachment 128207 [details] [review]
Optimization to load only needed .pc files.

Just a small change. Improved packages_foreach a little.

Please, confirm issues have been addressed.

Thanks!

Created attachment 128210 [details]
Bash script to benchmark pkg-config

Hi Dan!

I've just run a performance benchmark to check how faster pkg-config is after applying the patch.

The attached file is a bash script I created to benchmark it. The result is quite encouraging. When run with the patch applied, I get:

marco@marco-Inspiron-3421:~/dev/pkg-config$ time ./pkg_list > /dev/null

real	0m1.130s
user	0m0.208s
sys	0m0.340s

While when run pkg-config from distro, the result I get is:

marco@marco-Inspiron-3421:~/dev/pkg-config$ time ./pkg_list > /dev/null

real	0m1.993s
user	0m0.548s
sys	0m0.952s

The script simulates the most common use of pkg-config: many calls for different packages, such as when running a ./configure script. I'd say that getting the execution to 1.130 down from 1.993 is a huge performance win.

Just to confirm, when I checkout 091722179bda0a6dcf388fbfdfcd948b1cb53e9f (the commit just before my changes), the improvement is still very significant:

marco@marco-Inspiron-3421:~/dev/pkg-config$ time ./pkg_list > /dev/null

real	0m1.669s
user	0m0.328s
sys	0m0.908s


So, I really think we've got a measurable improvement here and would like to have the patch accepted.

Thank you!

Pushed to master in c53385b. I fixed up some indentation problems from
your latest version. I did some performance tests myself and do see a
nice win. Thanks for working on this!