diff mbox series

generic/478: why eval is so fast?

Message ID 817817a1-2f2d-1ee1-0898-f86801586d65@yandex.ru (mailing list archive)
State New, archived
Headers show
Series generic/478: why eval is so fast? | expand

Commit Message

stsp June 30, 2023, 9:50 a.m. UTC
Hi guys.

While playing with the test 478, I've
noticed something very strange. Namely,
this small patch, that only adds eval to
the script:



speeds up the test by 50-60%!
This is quite unbelievable, and probably
means such change breaks something.
But I have no idea what and how.
Can anyone come up with a good guess?

Comments

Zorro Lang July 3, 2023, 3:05 p.m. UTC | #1
On Fri, Jun 30, 2023 at 02:50:47PM +0500, stsp wrote:
> Hi guys.
> 
> While playing with the test 478, I've
> noticed something very strange. Namely,
> this small patch, that only adds eval to
> the script:
> 
> diff --git a/tests/generic/478 b/tests/generic/478
> index 480762d2..45b801d0 100755
> --- a/tests/generic/478
> +++ b/tests/generic/478
> @@ -106,24 +106,24 @@ do_test()
>         # print options and getlk output for debug
>         echo $* >> $seqres.full 2>&1
>         # -s : do setlk
> -       $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> +       eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
>         # -g : do getlk
> -       $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> +       eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
>                 tee -a $seqres.full
>         wait $!

I didn't find out why it run faster, but I'm sure we can't merge a
patch which only say "it's faster, but don't know why". Especially
that "faster" is really unnormal.

One thing I'm confused is about that background process, if you add eval to
that line, can you sure the later "$!" (in "wait $!") is the t_ofd_locks
process number?

The second eval with "| tee -a  $seqres.full" confused me too, I'm not
so familar with eval, can't what kind of problem it brings in, but I
doubt something wrong at there.

Anyway, I can't merge a patch like this. But I don't mind if anyone
can answer this question :)

Thanks,
Zorro

> 
>         # add -F to clone with CLONE_FILES
>         soptions="$1 -F"
>         # with -F, new locks are always file to place
> -       $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> -       $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> +       eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> +       eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
>                 tee -a $seqres.full
>         wait $!
> 
>         # add -d to dup and close
>         soptions="$1 -d"
> -       $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> -       $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> +       eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> +       eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
>                 tee -a $seqres.full
>         wait $!
>  }
> 
> 
> speeds up the test by 50-60%!
> This is quite unbelievable, and probably
> means such change breaks something.
> But I have no idea what and how.
> Can anyone come up with a good guess?
>
stsp July 3, 2023, 3:12 p.m. UTC | #2
03.07.2023 20:05, Zorro Lang пишет:
> I didn't find out why it run faster, but I'm sure we can't merge a
> patch which only say "it's faster, but don't know why". Especially
> that "faster" is really unnormal.
>
> One thing I'm confused is about that background process, if you add eval to
> that line, can you sure the later "$!" (in "wait $!") is the t_ofd_locks
> process number?
>
> The second eval with "| tee -a  $seqres.full" confused me too, I'm not
> so familar with eval, can't what kind of problem it brings in, but I
> doubt something wrong at there.
>
> Anyway, I can't merge a patch like this. But I don't mind if anyone
> can answer this question :)
Hi, no, this wasn't a merge request, just
a question. But the problem is that I need
to merge another (not yet posted) patch
that needs 1 of these evals to properly
parse spaces in an arguments.
The eval I need, is only in a line with "| tee"
stuff, not the one for the process that would
be later wait'ed for.
Still, seeing such a huge speed-up, I was
afraid to post the patch itself, and decided
to ask first.

Anyway, the patch in question waits for
an approval of the semaphore fixing patch.
It adds a new test, but the synchronization
needs to work properly first.
diff mbox series

Patch

diff --git a/tests/generic/478 b/tests/generic/478
index 480762d2..45b801d0 100755
--- a/tests/generic/478
+++ b/tests/generic/478
@@ -106,24 +106,24 @@  do_test()
         # print options and getlk output for debug
         echo $* >> $seqres.full 2>&1
         # -s : do setlk
-       $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
+       eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
         # -g : do getlk
-       $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
+       eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
                 tee -a $seqres.full
         wait $!

         # add -F to clone with CLONE_FILES
         soptions="$1 -F"
         # with -F, new locks are always file to place
-       $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
-       $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
+       eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
+       eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
                 tee -a $seqres.full
         wait $!

         # add -d to dup and close
         soptions="$1 -d"
-       $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
-       $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
+       eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
+       eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
                 tee -a $seqres.full
         wait $!
  }