Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CanonicalizePath: Remove kMaxComponents limit #2358

Merged
merged 1 commit into from
Dec 28, 2023

Conversation

digit-google
Copy link
Contributor

This patch refactors the CanonicalizePath() function to fix two issues and improve performance. This is achieved through the following:

  • Remove the kMaxComponents limit entirely, which fixes choking on lots of short paths (ninja: fatal: path has too many components) #1732, by dropping the components array entirely, in favor of back-tracking the destination pointer.

  • Properly handle / and \\ which were incorrectly converted into an empty string. This fixes ninja thinks the root directory doesn't exist #2008.

  • Skip initial ../ components in relative paths, as these are common when referencing source files in build plans, and doing so make the loop after this step run faster in practice since most source files do not need adjustments.

  • Simplify the inner loop logic by handling the last component (which is not followed by a trailing directory separator) separately.

    This noticeably improves performance because the inner loop becomes smaller with less branch mis-predictions in the general case.

  • Never access or copy the character after the end of the input string.

  • Use memchr() to find the next / on Posix, which allows the use of SIMD implementations provided by the C runtime (e.g. through IFUNC functions on Linux), resulting in very noticeable speedup. This is also why a statically Ninja executable will be slower than one that links to the C library dynamically :-/

  • Avoid performing any writes when the input path doesn't need any adjustment, which is also quite common.

Note that this patch does not remove the 64-bit limit for the slash_bits value, which is only used on Win32.

Benchmarking was done in several ways:

  • On Linux, running hyperfine canon-perftest to run the canonicalization benchmark program and measure its total running time. Three compilers were used to generate dynamically-linked executables.

                 BEFORE (ms)  AFTER (ms)
    
    GCC 13.2.0      651         369
    Clang 14.0      591         402
    Clang 18,0      653         400
    
  • On Windows, running canon-perftest 5 times and keeping the best reported average result. The number are slower since they only measure the benched function.

                    BEFORE (ms)  AFTER (ms)
    
    Mingw64 GCC 12      246         195
    
  • On Linux, run hyperfine ninja -C out/default -n --quiet on a large Fuchsia build plan, once with 70000+ pending commands, and once after the build (i.e. ninja: no work to do).

                BEFORE (s)  AFTER (s)
    
    pre_build       8.789    8.647
    post_build      6.703    6.590
    ```
    

@digit-google
Copy link
Contributor Author

Note that this PR replaces and deprecates #2108 that I had uploaded a long time ago to only fix one of the issues handled here.

src/util_test.cc Show resolved Hide resolved
src/util.cc Outdated Show resolved Hide resolved
This patch refactors the CanonicalizePath() to fix
two issues and improve performance. This is achieved
through the following:

- Remove the kMaxPathComponents limit entirely,
  which fixes ninja-build#1732, by dropping the
  `components` array entirely, in favor of
  back-tracking the destination pointer.

- Properly handle '/' and '\' which were incorrectly
  converted into an empty string. This fixes
  ninja-build#2008.

- Skip initial '../' components in relative paths,
  as these are common when referencing source files
  in build plans, and doing so make the loop after
  this step run faster in practice since most
  source files do not need adjustments.

- Simplify the inner loop logic by handling the
  last component (which is not followed by a
  trailing directory separator) separately.

  This noticeably improves performance because
  the inner loop becomes smaller with less branch
  mis-predictions in the general case.

- Never access or copy the caharacter after the
  end of the input string.

- Use memchr() to find the next '/' on Posix, which
  allows the use of SIMD implementations provided by
  the C runtime (e.g. through IFUNC functions on Linux),
  resulting in very noticeable speedup. This is also
  why a statically Ninja executable will be slower
  than one that links to the C library dynamically :-/

- Avoid performing any writes when the input path
  doesn't need any adjustment, which is also quite
  common.

Note that this patch does _not_ remove the 64-bit
limit for the `slash_bits`  value, which is only
used on Win32.

Benchmarking was done in several ways:

- On Linux, running `hyperfine canon-perftest` to run the
  canonicalization benchmark program and measure its total
  running time. Three compilers were used to generate
  dynamically-linked executables.

  ```
               BEFORE (ms)  AFTER (ms)

  GCC 13.2.0      651         369
  Clang 14.0      591         402
  Clang 18,0      653         400
  ```

- On Windows, running `canon-perftest` 5 times and keeping
  the best reported average result. The number are slower
  since they only measure the benched function.

  ```
                   BEFORE (ms)  AFTER (ms)

  Mingw64 GCC 12      246         195
  ```

- On Linux, run `hyperfine ninja -C out/default -n --quiet`
  on a large Fuchsia build plan, once with 70000+ pending
  commands, and once after the build (i.e. `ninja: no work to do`).

  ````
               BEFORE (s)  AFTER (s)

  pre_build       8.789    8.647
  post_build      6.703    6.590
  ```
@digit-google
Copy link
Contributor Author

Ping. I believe the requested changes have been applied 3 weeks ago. Let me know if you need more.

@jhasse jhasse merged commit 766eca4 into ninja-build:master Dec 28, 2023
10 checks passed
@digit-google digit-google deleted the fix-canonicalize-path2 branch January 11, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants