Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

[-- Attachment #1: Type: text/plain, Size: 3209 bytes --]

On Sun 2019-05-26 10:08:54 -0300, David Bremner wrote:
> This removes the dependency of this test script on gdb, and
> considerably speeds up the running of the tests.

This series looks good to me.  I've tested it with moreutils parallel
installed, and it reduces total CPU time for the parallel test suite
from ~76 seconds of CPU to ~56 seconds, a > %25 reduction in CPU cost
for the whole test suite, despite touching only one test.  This is
awesome work.

I also like the elegance of the proposed change.  It would be great to
see it extended to the rest of our test suite's dependency on GDB if we
can do it. (T050, T060, and T380 i think)

Details on my profiling:

On the current master (bc396c967c7cd8e7a109858e428d7bf97173f7a7),
without these changes applied, the whole test suite consumes this much
CPU on my 4-core intel i5-2540M (2.60GHz):

real	0m33.106s
user	1m1.150s
sys	0m14.998s

On the same machine, with the pair of patches applied, the whole test
suite consumes this:

real	0m27.557s
user	0m44.172s
sys	0m12.018s

Caveat: i don't know how well LD_PRELOAD works on non-GNU/Linux
platforms, and we're not using LD_PRELOAD elsewhere in the test suite
yet.  Perhaps Ralph Seichter (explicitly cc'ed above) could comment on
how it'll affect homebrew?  Do we need to consider a variant for
platforms that can't do LD_PRELOAD?

One nit-pick below:

> diff --git a/test/T070-insert.sh b/test/T070-insert.sh
> index 48165caa..ab26ecd4 100755
> --- a/test/T070-insert.sh
> +++ b/test/T070-insert.sh
> @@ -2,8 +2,6 @@
>  test_description='"notmuch insert"'
>  . $(dirname "$0")/test-lib.sh || exit 1
>  
> -test_require_external_prereq gdb
> -
>  # subtests about file permissions assume that we're working with umask
>  # 022 by default.
>  umask 022
> @@ -246,50 +244,38 @@ test_expect_code 1 "notmuch insert $gen_msg_filename 2>&1"
>  notmuch config set new.tags $OLDCONFIG
>  
>  # DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
> +gen_insert_msg
>  
> -for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
> -    READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
> -cat <<EOF > index-file-$code.gdb
> -set breakpoint pending on
> -set logging file index-file-$code.log
> -set logging on
> -break notmuch_database_index_file
> -commands
> -return NOTMUCH_STATUS_$code
> -continue
> -end
> -run
> +# pregenerate all of the test shims
> +for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR OUT_OF_MEMORY XAPIAN_EXCEPTION; do
> +    make_shim shim-$code <<EOF
> +#include <notmuch.h>
> +#include <stdio.h>
> +notmuch_status_t
> +notmuch_database_index_file (notmuch_database_t *notmuch,
> +                             const char *filename,
> +                             notmuch_indexopts_t *indexopts,
> +                             notmuch_message_t **message_ret)
> +{
> +  return NOTMUCH_STATUS_$code;
> +}
>  EOF
>  done
>  
> -gen_insert_msg
> -

Why put the shim generation below gen_insert_msg?  If it were above, it
would make the comment about DUPLICATE_MESSAGE_ID less confusing, and
the changeset could be shorter.

          --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]