Optimize `Wtf8Buf::into_string` for the case where it contains UTF-8. #96869

sunfishcode · 2022-05-09T14:52:21Z

Add a is_known_utf8 flag to Wtf8Buf, which tracks whether the
string is known to contain UTF-8. This is efficiently computed in many
common situations, such as when a Wtf8Buf is constructed from a String
or &str, or with Wtf8Buf::from_wide which is already doing UTF-16
decoding and already checking for surrogates.

This makes OsString::into_string O(1) rather than O(N) on Windows in
common cases.

And, it eliminates the need to scan through the string for surrogates in
Args::next and Vars::next, because the strings are already being
translated with Wtf8Buf::from_wide.

Many things on Windows construct OsStrings with Wtf8Buf::from_wide,
such as DirEntry::file_name and fs::read_link, so with this patch,
users of those functions can subsequently call .into_string() without
paying for an extra scan through the string for surrogates.

r? @ghost

library/std/src/sys_common/wtf8.rs

sunfishcode · 2022-05-11T21:42:58Z

r? rust-lang/libs

joshtriplett · 2022-05-17T22:03:53Z

@bors r+ rollup=never

bors · 2022-05-17T22:03:55Z

📌 Commit f4c93bcbe837ab5220beaa2892fc249f49a9e740 has been approved by joshtriplett

bors · 2022-05-17T22:34:11Z

⌛ Testing commit f4c93bcbe837ab5220beaa2892fc249f49a9e740 with merge 7f13587a59ff3c5350b77bf9334994f91f05d9fc...

bors · 2022-05-17T23:05:47Z

💔 Test failed - checks-actions

sunfishcode · 2022-05-18T01:30:11Z

src/librustdoc/html/render/context.rs had an assert that rustdoc's render context was a certain size. It's already not checked on most platforms, as it's just a guard against the context growing unexpectedly, so I've now added a patch to disable the check on Windows too.

bors · 2022-05-28T04:40:58Z

☔ The latest upstream changes (presumably #97433) made this pull request unmergeable. Please resolve the merge conflicts.

crlf0710 · 2022-05-28T05:38:38Z

There's merge conflict now. Needs a rebase.

sunfishcode · 2022-05-31T14:42:10Z

r? @joshtriplett

sunfishcode · 2022-05-31T17:01:15Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

joshtriplett · 2022-06-23T19:23:11Z

@bors r+

bors · 2022-06-23T19:23:12Z

📌 Commit e620b42644e3404377fa0fae0afd660c6bd77999 has been approved by joshtriplett

bors · 2022-06-23T19:23:13Z

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

klensy · 2022-06-23T20:06:53Z

Merge branch 'master' into main
https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy
There was warning for it previously?

Add a `is_known_utf8` flag to `Wtf8Buf`, which tracks whether the string is known to contain UTF-8. This is efficiently computed in many common situations, such as when a `Wtf8Buf` is constructed from a `String` or `&str`, or with `Wtf8Buf::from_wide` which is already doing UTF-16 decoding and already checking for surrogates. This makes `OsString::into_string` O(1) rather than O(N) on Windows in common cases. And, it eliminates the need to scan through the string for surrogates in `Args::next` and `Vars::next`, because the strings are already being translated with `Wtf8Buf::from_wide`. Many things on Windows construct `OsString`s with `Wtf8Buf::from_wide`, such as `DirEntry::file_name` and `fs::read_link`, so with this patch, users of those functions can subsequently call `.into_string()` without paying for an extra scan through the string for surrogates.

…case`.

This assert is just making sure the size of `Context` doens't grow unexpectedly, and it's already not being checked on every platform. `PathBuf` now has a different size on Windows, so adjust this to avoid checking the size on Windows.

sunfishcode · 2022-06-23T20:14:36Z

I think I used the github merge-conflict UI and didn't realize that generated a merge commit. I've now fixed that.

r? @joshtriplett

sunfishcode · 2022-08-23T23:13:14Z

Ping @joshtriplett; this is already r+ above, it just needs an approval because of the merge-commit issue which is now fixed.

jyn514 · 2022-08-23T23:16:59Z

@bors r=joshtriplett

bors · 2022-08-23T23:17:01Z

📌 Commit ddeb936 has been approved by joshtriplett

It is now in the queue for this repository.

bors · 2022-08-24T01:17:55Z

⌛ Testing commit ddeb936 with merge 25ea5a3...

bors · 2022-08-24T04:06:46Z

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 25ea5a3 to master...

rust-timer · 2022-08-24T05:22:17Z

Finished benchmarking commit (25ea5a3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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.

	mean¹	range	count²
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	4.9%	[2.6%, 7.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-3.9%, -2.0%]	4
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

the arithmetic mean of the percent change ↩
number of relevant changes ↩

RalfJung · 2024-04-26T16:04:06Z

library/std/src/sys_common/wtf8.rs

+    /// It is possible for `bytes` to have valid UTF-8 without this being
+    /// set, such as when we're concatenating `&Wtf8`'s and surrogates become
+    /// paired, as we don't bother to rescan the entire string.
+    is_known_utf8: bool,


Adding a field here introduced a subtle bug in PathBuf: #124409.

Privacy-breaking transmutes are "fun". ;)

…ng-utf8-invariant, r=<try> Make PathBuf less Ok with adding UTF-16 then `into_string` Fixes rust-lang#126291 which is, as far as I can tell, a regression introduced by rust-lang#96869. try-job: x86_64-msvc

…ring-utf8-invariant, r=joboet Make PathBuf less Ok with adding UTF-16 then `into_string` Fixes rust-lang#126291 which is, as far as I can tell, a regression introduced by rust-lang#96869. try-job: x86_64-msvc

Rollup merge of rust-lang#126305 - workingjubilee:fix-os-string-to-string-utf8-invariant, r=joboet Make PathBuf less Ok with adding UTF-16 then `into_string` Fixes rust-lang#126291 which is, as far as I can tell, a regression introduced by rust-lang#96869. try-job: x86_64-msvc

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 9, 2022

sunfishcode mentioned this pull request May 9, 2022

Document that OsString and OsStr are bytes; provide conversions to bytes #95290

Closed

jyn514 reviewed May 9, 2022

View reviewed changes

library/std/src/sys_common/wtf8.rs Outdated Show resolved Hide resolved

library/std/src/sys_common/wtf8.rs Show resolved Hide resolved

library/std/src/sys_common/wtf8.rs Show resolved Hide resolved

library/std/src/sys_common/wtf8.rs Outdated Show resolved Hide resolved

sunfishcode marked this pull request as ready for review May 10, 2022 14:28

rust-highfive assigned joshtriplett May 11, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 17, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 17, 2022

This comment has been minimized.

Sign in to view

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2022

sunfishcode added 5 commits June 23, 2022 13:10

Panic safety.

d3a585e

Don't eagerly scan for is_known_utf8 in to_ascii_lowercase/`upper…

a7d57c6

…case`.

Remove is_known_utf8 checks from more tests where it's no longer set.

516a67a

sunfishcode force-pushed the main branch from e620b42 to ddeb936 Compare June 23, 2022 20:13

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 10, 2022

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2022

bors merged commit 25ea5a3 into rust-lang:master Aug 24, 2022

rustbot added this to the 1.65.0 milestone Aug 24, 2022

sunfishcode deleted the main branch August 24, 2022 04:07

bors mentioned this pull request Aug 24, 2022

Rustdoc : stop cloning html render context #100271

Closed

RalfJung reviewed Apr 26, 2024

View reviewed changes

workingjubilee mentioned this pull request Jun 12, 2024

Make PathBuf less Ok with adding UTF-16 then into_string #126305

Merged

workingjubilee mentioned this pull request Jun 12, 2024

Remove internal PathBuf::as_mut_vec #126333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize `Wtf8Buf::into_string` for the case where it contains UTF-8. #96869

Optimize `Wtf8Buf::into_string` for the case where it contains UTF-8. #96869

sunfishcode commented May 9, 2022

sunfishcode commented May 11, 2022

joshtriplett commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

This comment has been minimized.

sunfishcode commented May 18, 2022

bors commented May 28, 2022

crlf0710 commented May 28, 2022

sunfishcode commented May 31, 2022

sunfishcode commented May 31, 2022

joshtriplett commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

klensy commented Jun 23, 2022

sunfishcode commented Jun 23, 2022

sunfishcode commented Aug 23, 2022

jyn514 commented Aug 23, 2022

bors commented Aug 23, 2022

bors commented Aug 24, 2022

bors commented Aug 24, 2022

rust-timer commented Aug 24, 2022

RalfJung Apr 26, 2024 •

edited

Loading

Optimize Wtf8Buf::into_string for the case where it contains UTF-8. #96869

Optimize Wtf8Buf::into_string for the case where it contains UTF-8. #96869

Conversation

sunfishcode commented May 9, 2022

sunfishcode commented May 11, 2022

joshtriplett commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

This comment has been minimized.

sunfishcode commented May 18, 2022

bors commented May 28, 2022

crlf0710 commented May 28, 2022

sunfishcode commented May 31, 2022

sunfishcode commented May 31, 2022

joshtriplett commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

klensy commented Jun 23, 2022

sunfishcode commented Jun 23, 2022

sunfishcode commented Aug 23, 2022

jyn514 commented Aug 23, 2022

bors commented Aug 23, 2022

bors commented Aug 24, 2022

bors commented Aug 24, 2022

rust-timer commented Aug 24, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

RalfJung Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Optimize `Wtf8Buf::into_string` for the case where it contains UTF-8. #96869

Optimize `Wtf8Buf::into_string` for the case where it contains UTF-8. #96869

RalfJung Apr 26, 2024 •

edited

Loading