Use posix_spawn for absolute paths on macOS

[sunshowers]

Currently, on macOS, Rust never uses the fast posix_spawn path if a
directory change is requested, due to a bug in Apple's libc. However, the
bug is only triggered if the program is a relative path.

This PR makes it so that the fast path continues to work if the program
is an absolute path or a lone filename.

This was an alternative proposed in #80537 (comment), and it makes a measurable performance difference in some of my code that spawns thousands of processes.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

[thomcc]

Thanks, glad to see fixes that make us use posix_spawn over fork/exec more frequently.

I actually reported the issue this works around with apple a while ago, no luck for them fixing it tho.... There are a few other workarounds we could do in the relative path case, but I don't know if it's worth it in practice. Interesting to hear it is for your case.

(iffy because platform specific, and has some impact on perf even tho only on macOS hopefully -- altho I suppose this does the path scanning on all targets...)

@bors r+ rollup=iffy

[bors]

📌 Commit 692717e9decb9d3a2d2d70ec7201a62c57342cda has been approved by thomcc

It is now in the queue for this repository.

[sunshowers]

Testing this out on an M1 Mac Mini with cargo-nextest 0.9.35, on the clap repo at c023db3a98d1de3ca7067f92f31ba8109d954b99 with command:

cargo nextest run -E 'not (test(ui_tests) + test(example_tests))'

I get (best of 3):

Before:

Summary [   0.612s] 860 tests run: 860 passed, 2 skipped

After:

Summary [   0.314s] 860 tests run: 860 passed, 2 skipped

So there's a pretty substantial perf improvement, around 49% (1.94x).

[thomcc]

Wow, that's huge (especially for a real-world usage and not a process spawn time microbenchmark...). It might be worth converting the relative path to absolute too to get similar benefits, but might be too error-prone. Any thoughts there?

(There also are undocumented APIs we can use for this -- pthread_chdir_np can override the cwd just for the current thread, so we could do this, posix_spawn, then remove the thread's cwd override; chromium does this, so it's unlikely to be removed anytime soon, but we probably don't want to use it in the stdlib, so that code using those that won't get patches still is likely to work as far into the future as possible)

[sunshowers]

My feeling is that converting relative paths to absolute ones might just be a little too magical.

Wonder how much sense it would make to write some non-stability-guarantee documentation around this. Basically explaining the whole situation around posix_spawn and fork/exec, then describing how to achieve best results.

[sunshowers]

Oh, and one thing to note is that the example I picked is definitely realistic, but it is pretty much the best case for this perf improvement—lots of really small tests. For other workspaces with longer-running tests, the improvement won't be nearly as drastic.

[bors]

⌛ Testing commit 692717e9decb9d3a2d2d70ec7201a62c57342cda with merge c67dfe869d22937f082fbf2ed34ec6dcd2a25ac1...

[bors]

💔 Test failed - checks-actions

[sunshowers]

I guess this commit needs to be reapproved?

Is there a way to have a shorter turnaround time than a week to get CI feedback? :)

[thomcc]

Most of the issue here is due to the github timeouts we had recently causing quite a bit of backlog. We're only just getting through them. I also did mark this as iffy, which means it generally won't have as high throughput as normal. Anyway,

@bors delegate+

You can just bors r+ it if you want, or if you're concerned with further failures, you can use https://rustc-dev-guide.rust-lang.org/tests/ci.html in conjunction with bors try (but obviously don't land any changes to ci in this patch)

[bors]

✌️ @sunshowers can now approve this pull request

[sunshowers]

@bors r+

[bors]

📌 Commit bd8b4b9 has been approved by sunshowers

It is now in the queue for this repository.

[bors]

⌛ Testing commit bd8b4b9 with merge 7a42ca9...

[bors]

☀️ Test successful - checks-actions
Approved by: sunshowers
Pushing 7a42ca9 to master...

[rust-timer]

Finished benchmarking commit (7a42ca9): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	3.2%	[3.2%, 3.2%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	3.7%	[2.1%, 5.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[-2.0%, 5.3%]	3

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[thomcc]

My feeling is that converting relative paths to absolute ones might just be a little too magical.

Yes, this is probably true, since it's observably argv[0].

FWIW, I reported the macOS bug around relative paths to Apple last week (and even took the time to write a 10 line C program that demonstrates the problem, and point them at the buggy lines of code in their open source libc repo), so hopefully they'll fix it and we'll be able to (after a version check) use posix_spawnp even on relative paths.