Re: ??? Re: [PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation

[Kjetil Barvik <barvik@broadpark.no>]

* Kjetil Barvik <barvik@broadpark.no> writes:
> +	/*
> +	 * Is the cached path string a substring of 'name', is 'name'
> +	 * a substring of the cached path string, or is 'name' and the
> +	 * cached path string the exact same string?
> +	 */
> +	if (i >= max_len && ((i < len && name[i] == '/') ||
> +			     (i < cache.len && cache.path[i] == '/') ||
> +			     (len == cache.len))) {

* Junio C Hamano <gitster@pobox.com> writes:
> As you described in your commit log message, what this really wants to
> check is (i == max_len).  By saying (i >= max_len) you are losing
> readability, optimizing for compilers (that do not notice this) 

  When the compiler see the source line:

    'while (i < max_len && name[i] == cache.path[i])'

  it will, from what I know about compilers and machine instructions for
  intel cpu's, generate the following pseudo instructions for this line
  (before more compiler optimisation is done):

   1    test !(i < max_len)   /* which is (i >= max_len) */
   2    jump-if-not-zero  first_instruction_address_outside_the_loop
   3    "some instructions to retrieve some memory locations and test
         name[i] == cache.path[i]"
   4    jump-if-not-zero instruction-address-to-continue-to-loop

  So, I thought that if I could use the exact same test as the compiler
  have to generate for line 1, then I would maybe saved a test, and
  maybe also a jump instruction.

  Compiling with 'gcc -Os' (optimise for text segment size) I was able to
  save 4 bytes for this file, which I think shows that gcc is able to
  take advantage of this trick.

> and pessimizing for human readers (like me, who had to spend a few
> dozens of seconds to realize what you are doing, to speculate why you
> might have thought this would be a good idea, and writing this
> paragraph).  I do not know if it is a good trade-off.

  But, it was not easy to come up with a real test which shows a
  noticeable time difference from this trick, so, I guess this is only a
  trick for the kernel people (at most), so I revert it and will use
  '==' for version 2, such that we keep a little better readability.

  -- kjetil

  ps! Compiling with '-march=core2 -O2 -g0 -s -fomit-frame-pointer', the
      difference in text segment was 64 bytes when using this trick.