Skip to content

try: remove define_opaque from unix split_paths - #163569

Closed
CAD97 wants to merge 1 commit into
rust-lang:mainfrom
CAD97:is-define-opaque-slow
Closed

CAD97 wants to merge 1 commit into
rust-lang:mainfrom
CAD97:is-define-opaque-slow

Conversation

@CAD97

@CAD97 CAD97 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

#158936 had a regression in the serde benchmark doc profile. The only hypothesis I have as to how is that potentially the usage of #[define_opaque] is slow. This manually defines the #[define_opaque] in std::sys::paths::unix to test that hypothesis.

r? @JonathanBrouwer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2026
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 30, 2026
@rustbot

rustbot commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

JonathanBrouwer is currently at their maximum review capacity.
They may take a while to respond.

@JonathanBrouwer

Copy link
Copy Markdown
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 30, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Sep 30, 2026
try: remove define_opaque from unix split_paths
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job test-pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] object test:false 8.903
error: unneeded `return` statement
  --> library/std/src/sys/paths/unix.rs:83:13
   |
83 |             return Some(Path::new(OsStr::from_bytes(split.0)));
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/main/index.html#needless_return
   = note: requested on the command line with `-D clippy::needless-return`
help: remove `return`
   |
83 -             return Some(Path::new(OsStr::from_bytes(split.0)));
83 +             Some(Path::new(OsStr::from_bytes(split.0)))
   |

error: unneeded `return` statement
  --> library/std/src/sys/paths/unix.rs:86:13
   |

Comment on lines +83 to +86
return Some(Path::new(OsStr::from_bytes(split.0)));
} else {
self.unparsed = None;
return Some(Path::new(OsStr::from_bytes(unparsed)));

@CAD97 CAD97 Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix the clippy::needless-return error:

Suggested change
return Some(Path::new(OsStr::from_bytes(split.0)));
} else {
self.unparsed = None;
return Some(Path::new(OsStr::from_bytes(unparsed)));
Some(Path::new(OsStr::from_bytes(split.0)));
} else {
self.unparsed = None;
Some(Path::new(OsStr::from_bytes(unparsed)));

Not committing that as I don't know whether the try build cares

View changes since the review

@rust-bors

rust-bors Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: b82ccf1 (b82ccf19f74e8f5e8773a1a9e84c7c2a90ab7474)
Base parent: 21b707e (21b707e3f97e0b522ebd2f277a862339625ad83f)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (b82ccf1): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.4%, -0.3%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 3.0%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
3.0% [3.0%, 3.0%] 1
Regressions ❌
(secondary)
3.8% [3.8%, 3.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) 3.0% [3.0%, 3.0%] 1

Cycles

Results (primary -3.2%, secondary -5.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
26.6% [26.6%, 26.6%] 1
Improvements ✅
(primary)
-3.2% [-3.2%, -3.2%] 1
Improvements ✅
(secondary)
-20.7% [-21.4%, -20.0%] 2
All ❌✅ (primary) -3.2% [-3.2%, -3.2%] 1

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.2%, -0.0%] 88
Improvements ✅
(secondary)
-0.1% [-0.2%, -0.0%] 57
All ❌✅ (primary) -0.1% [-0.2%, -0.0%] 88

Bootstrap: 487.961s -> 489.948s (0.41%)
Artifact size: 406.51 MiB -> 406.41 MiB (-0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 30, 2026
@CAD97

CAD97 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

neutral result on different benchmarks

@CAD97 CAD97 closed this Oct 1, 2026
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants