[asder8215]

View all comments

This PR fixes #154521 specifically for Unix/non-prefixed platforms. By non-prefixed filepath platforms this includes:

• all(target_vendor = "fortanix", target_env = "sgx")
• not(any(target_os = "cygwin", target_os = "uefi", target_os = "solid_asp3", target_os = "windows") (the default/fallback _ path in sys/path/mod.rs, which notably includes Unix platforms)

For every other prefixed filepath platforms (notably Windows), the Components iterator implementation remains largely unchanged (just some minor refactoring).

The point of the non-prefixed Components implementation is to:

1. Gravitate away from accounting for the Prefix components that Windows/other prefixed-based filepath platform use as Unix-based platforms do not have those prefix components in their filepaths;
2. Optimize the whole Components iterator traversal algorithm as well as improve Path equality and Path comparison

From benchmarking, I noticed that all the Prefix checking/returning code specific for Windows affects how Components perform on Unix, though to be fair, part of the reason that is the case is because the Prefix enum takes up 40 bytes to represent (which we shouldn't want Unix to take the consequence of handling Prefix). There are other factors in the current Components::next/Components::next_back implementation that could've been optimized better or written in a way that would enable the compiler to optimize that code better (e.g. autovectorization optimizations). It's certainly possible that the prefixed/current Components implementation could be enhanced to make performance better on Windows, but I will leave that to someone who can verifiably test and benchmark those changes on Windows.

In terms of the optimizations I made for the non-prefixed Components, a smaller one I could talk about first is that the size of Components has been reduced. In the current/original Components we have this:

#[derive(Copy, Clone, PartialEq, PartialOrd, Debug)]
enum State {
    Prefix = 0,   // c:
    StartDir = 1, // / or . or nothing
    Body = 2,     // foo/bar/baz
    Done = 3,
}

#[derive(Clone)]
pub struct Components<'a> {
    // The path left to parse components from
    path: &'a [u8],

    // The prefix as it was originally parsed, if any
    prefix: Option<Prefix<'a>>,

    // true if path *physically* has a root separator; for most Windows
    // prefixes, it may have a "logical" root separator for the purposes of
    // normalization, e.g., \\server\share == \\server\share\.
    has_physical_root: bool,

    // The iterator is double-ended, and these two states keep track of what has
    // been produced from either end
    front: State,
    back: State,
}

This overall takes up 64 bytes to represents Components

Now, without the need to account for Prefix, we can simplify the Components struct to:

#[derive(Copy, Clone, PartialEq, PartialOrd, Debug)]
enum State {
    Absolute = 1, // A root component (i.e. '/')
    Relative = 2, // A relative component ('foo')
    Done = 3,     // Iterator is fully consumed
}

#[derive(Clone)]
pub struct Components<'a> {
    // The path left to parse components from
    path: &'a [u8],
    // The current state of the iterator
    state: State,
}

This prefix-less Components takes up 24 bytes to represent.

Implementation-wise, Components::next and Components::next_back takes a similar approach to the original Components iterator by subslicing the path field to present the left over unconsumed path. However, one of the key differences with this Components::next and Components::next_back is that we normalize away in between and trailing separator + cur dir bytes after parsing the component (e.g. "foo///bar", when we parse "foo" via Components::next or "bar" via Components::next_back", the in between separator bytes will be subsliced from the path field in preparation for the next Components::next/Components::next_back). Normalizing away these bytes in preparation for the next Components::next/Components::next_back were not done in the original implementation, which meant that while !self.is_finished loop in the original ran through multiple iterations to actually normalize away those separator bytes and find the next component to parse. Moreover, I relied on iterator methods to make that normalization process really fast, as it appears to be autovectorized and optimized really well by the compiler. Also noteworthy is that the optimization made to normalizing away in between/trailing separator/cur dir bytes using iterator methods enhances the performance of Components::as_path significantly.

As for Path equality and Path comparison, which largely rely on Components underneath the hood, there are many optimizations we introduce there to increase performance.

Starting with Path equality, we check for the following:

• Introduced by @Lfan-ke, we check directly if the two Paths when converted as an OsStrs are equal. If so, we can just return true immediately. Otherwise, we have to hinge on Components (e.g. of "/foo" is equivalent to "/foo/").
• We have a fast path in checking if either the left path or right path an empty path? We can easily use Components state field to determine if we're looking at an empty path (reading 1 byte via the State::Done enum rather than directly looking at each path's length)
• We use iterator methods to check for the first mismatching byte between the left and right path. So long as the mismatching byte is not a separator or cur dir byte on either end, we can directly return false.
• Otherwise, we find the nearest separator byte from the right (also using iterator methods) for both to readjust their Components path field and just rely on Components::next_back to handle the rest on determining whether the two paths are equivalent or not.

As for Path comparison, we do the following:

• Check if the two paths are equal. If so, we can return Ordering::Equal immediately.
• We then use iterator methods to check for the first mismatching byte between the left and right path. So long as the mismatching byte is not a separator or cur dir byte on either end, we can directly compare the byte and return Ordering::Greater or Ordering::Less as appropriate).
• Otherwise, we find the nearest separator byte from the left (also using iterator methods) for both to readjust their Components path field and just rely on Components::next to handle the rest on determining whether to return Ordering::Equal, Ordering::Less, or Ordering::Greater.
  • Note: this was already done from the original Components implementation, so this code/idea was just taken from there.

All of these enhances the performance of Path equality and comparison significantly, which you can see from benchmarking results.

Regarding benchmarking, I did all benchmarking and experimentation on this Components implementation within a separate repository. You can take a look here to see the raw and markdown formatted results and the methodology I chose to go about benchmarking this Components iterator implementation.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 8 candidates

[asder8215]

New benchmarking results. You can see what the benchmark code looks like here and run it yourself to see if there are any difference in measurements on your end:

This is the measurement of the current implementation of Components<'_> (without black box):

Std Components (No BB)  time:   [21.546 µs 21.800 µs 22.096 µs]
Found 5 outliers among 100 measurements (5.00%)
  4 (4.00%) high mild
  1 (1.00%) high severe

Std Components Next (No BB)
                        time:   [20.434 µs 20.482 µs 20.538 µs]
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

Std Components Next Back (No BB)
                        time:   [38.367 µs 38.757 µs 39.199 µs]
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

Std Path Iter (No BB)   time:   [21.547 µs 21.730 µs 21.921 µs]

Std As Path Iter (No BB)
                        time:   [87.680 µs 88.439 µs 89.231 µs]
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe

Std Eq Comps (No BB)    time:   [591.21 ns 593.35 ns 595.82 ns]
Found 16 outliers among 100 measurements (16.00%)
  1 (1.00%) low severe
  3 (3.00%) low mild
  7 (7.00%) high mild
  5 (5.00%) high severe

Std Uneq Comps (No BB)  time:   [60.953 ns 61.419 ns 61.911 ns]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

Std Uneq 2 Comps (No BB)
                        time:   [75.454 µs 75.734 µs 76.027 µs]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

Std Compare Comps (No BB)
                        time:   [46.182 µs 46.621 µs 47.192 µs]
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

Std Compare Uneq Comps (No BB)
                        time:   [46.679 µs 46.980 µs 47.291 µs]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

Std Compare Uneq 2 Comps (No BB)
                        time:   [41.480 ns 41.827 ns 42.160 ns]
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

This is the measurement of the new implementation of Components<'_> I'm working on (without black box):

Components Rewrite (No BB)
                        time:   [24.982 µs 25.267 µs 25.570 µs]

Components Next Rewrite (No BB)
                        time:   [24.388 µs 24.655 µs 24.937 µs]
Found 6 outliers among 100 measurements (6.00%)
  6 (6.00%) high mild

Components Next Back Rewrite (No BB)
                        time:   [18.184 µs 18.567 µs 19.034 µs]
Found 16 outliers among 100 measurements (16.00%)
  1 (1.00%) high mild
  15 (15.00%) high severe

Path Iter Rewrite (No BB)
                        time:   [23.485 µs 23.659 µs 23.829 µs]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

As Path Iter Rewrite (No BB)
                        time:   [22.936 µs 23.066 µs 23.208 µs]
Found 7 outliers among 100 measurements (7.00%)
  3 (3.00%) high mild
  4 (4.00%) high severe

Eq Comps Rewrite (No BB)
                        time:   [605.12 ns 608.83 ns 612.98 ns]
Found 11 outliers among 100 measurements (11.00%)
  1 (1.00%) low mild
  7 (7.00%) high mild
  3 (3.00%) high severe

Uneq Comps Rewrite (No BB)
                        time:   [31.799 ns 32.108 ns 32.433 ns]
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild

Uneq Comps 2 Rewrite (No BB)
                        time:   [47.091 µs 48.186 µs 49.085 µs]

Compare Comps Rewrite (No BB)
                        time:   [50.234 µs 50.725 µs 51.254 µs]
Found 10 outliers among 100 measurements (10.00%)
  9 (9.00%) high mild
  1 (1.00%) high severe

Compare Uneq Comps Rewrite (No BB)
                        time:   [49.262 µs 49.631 µs 50.067 µs]
Found 16 outliers among 100 measurements (16.00%)
  4 (4.00%) high mild
  12 (12.00%) high severe

Compare Uneq Comps 2 Rewrite (No BB)
                        time:   [43.397 ns 43.767 ns 44.171 ns]
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild

Edit: Updated Components::as_path to match on Option<FirstComponent>/self.first_comp instead of using if let Some(_) = self.first_comp and matching on that, benchmarking for this PR Components<'_> has been updated as a result. Everything else is unaffected by this change.

[asder8215]

Here are the benchmark results with black box:

From current Components<'_> implementation:

Std Components          time:   [20.947 µs 21.010 µs 21.084 µs]
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe

Std Components Next     time:   [20.967 µs 20.993 µs 21.021 µs]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

Std Components Next Back
                        time:   [35.715 µs 35.802 µs 35.925 µs]
Found 20 outliers among 100 measurements (20.00%)
  6 (6.00%) high mild
  14 (14.00%) high severe

Std Path Iter           time:   [20.883 µs 20.992 µs 21.152 µs]
Found 12 outliers among 100 measurements (12.00%)
  5 (5.00%) high mild
  7 (7.00%) high severe

Std As Path Iter        time:   [80.673 µs 80.935 µs 81.261 µs]
Found 9 outliers among 100 measurements (9.00%)
  6 (6.00%) high mild
  3 (3.00%) high severe

Std Eq Comps            time:   [589.43 ns 593.36 ns 597.88 ns]
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) low severe
  2 (2.00%) low mild
  1 (1.00%) high severe

Std Uneq Comps          time:   [63.919 ns 64.262 ns 64.765 ns]
Found 10 outliers among 100 measurements (10.00%)
  6 (6.00%) high mild
  4 (4.00%) high severe

Std Uneq 2 Comps        time:   [75.284 µs 75.939 µs 76.599 µs]
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high severe

From this Components<'_> implementation PR:

Components Rewrite      time:   [24.190 µs 24.425 µs 24.687 µs]
Found 5 outliers among 100 measurements (5.00%)
  4 (4.00%) high mild
  1 (1.00%) high severe

Components Next Rewrite time:   [24.230 µs 24.550 µs 24.889 µs]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

Components Next Back Rewrite
                        time:   [17.339 µs 17.488 µs 17.655 µs]
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

Path Iter Rewrite       time:   [23.845 µs 23.996 µs 24.154 µs]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

As Path Iter Rewrite    time:   [22.431 µs 22.676 µs 23.010 µs]
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe

Eq Comps Rewrite        time:   [586.16 ns 588.10 ns 590.14 ns]

Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) low mild
  2 (2.00%) high mild
  1 (1.00%) high severe

Uneq Comps Rewrite      time:   [31.733 ns 32.023 ns 32.378 ns]

Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) high mild
  6 (6.00%) high severe

Uneq 2 Comps Rewrite    time:   [36.318 µs 36.574 µs 36.913 µs]
Found 23 outliers among 100 measurements (23.00%)
  23 (23.00%) high severe

Edit: Updated Components::as_path to match on Option<FirstComponent>/self.first_comp instead of using if let Some(_) = self.first_comp and matching on that, benchmarking for this PR Components<'_> has been updated as a result. Everything else is unaffected by this change.

Edit 2: Took off Path ordering benchmark here since it was incorrect see below to see corrected path ordering benchmarks.

[asder8215]

I'm confident this code works (passed CI in previous run, the current amended commit change I made doesn't change logic, but makes the code written in a more idiomatic way).

In my opinion, the logic in this code should look more readable than how Component<'_> is currently written implemented as. From benchmarking, we can see that Components::next_back, Components::as_path, and in the cases where Components equality falls down to using Components::next_back (when they are unequal or equality can't be determine unless one/both Components<'_> are normalized), this PR implementation of Components<'_> is faster than how it's currently implemented as. The trade off is that this PR implementation of Components<'_> has a slight reduction in performance for Components::next and as a result Components<'_> comparison, but I would take this slight reduction in performance to make path equality faster.

[asder8215]

@rustbot label +I-libs-nominated

Since Components<'_> is pretty well-used in many other methods, I think this may need discussion from the libs team on whether the re-implementation of Components<'_> here is okay/valid to take over the current implementation (and the trade-off between faster Components::next_back with a slight reduction in performance in Components::next). I wasn't sure if this should be labeled as I-libs-api-nominated or not since it pertains to an existing stable feature than a new feature.

[rust-bors]

💔 Test for 86c1566 failed: CI. Failed job:

• Calculate job matrix (web logs, enhanced plaintext logs)

[asder8215]

@bors try jobs=test-x86_64-msvc-,test-i686-msvc,test-aarch64-msvc-

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0b71db5 (0b71db5cb857bee9d456cdbf2f0a028f4d1d5c04)
Base parent: 5d89371 (5d8937173e7c40406c167f4e94737da8925333ad)

[asder8215]

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.