Repository navigation
sort: use locale_cmp when all selectors have equal ordering - #13350
lauchimoon wants to merge 1323 commits into
Conversation
Merging this PR will regress 13 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | check_sorted_utf8_locale |
422.9 ms | 491.1 ms | -13.89% |
| ❌ | Simulation | sort_numeric_utf8_locale |
36.1 ms | 40.4 ms | -10.76% |
| ❌ | Simulation | merge_single_file_utf8_locale |
119.3 ms | 133.3 ms | -10.55% |
| ❌ | Simulation | expand_many_lines[100000] |
95 ms | 106.2 ms | -10.53% |
| ❌ | Simulation | five_38_bit_primes |
1.5 s | 1.7 s | -10.07% |
| ❌ | Simulation | expand_custom_tabstops[50000] |
26.9 ms | 29.6 ms | -9.27% |
| ❌ | Simulation | merge_pre_sorted_files_utf8_locale |
237.9 ms | 252 ms | -5.59% |
| ❌ | Simulation | merge_pre_sorted_files |
238.7 ms | 252.6 ms | -5.51% |
| ❌ | Simulation | sort_numeric[500000] |
2.5 s | 2.7 s | -5.43% |
| ❌ | Simulation | sort_german_de_locale |
282.1 ms | 295.3 ms | -4.49% |
| ❌ | Simulation | sort_case_insensitive[500000] |
480.6 ms | 501.7 ms | -4.21% |
| ❌ | Simulation | sort_case_sensitive[500000] |
321 ms | 334.9 ms | -4.14% |
| ❌ | Simulation | sort_spill_to_tmp_files_utf8_locale |
321 ms | 332.1 ms | -3.33% |
| ⚡ | Simulation | three_39_bit_primes |
901.6 ms | 616.4 ms | +46.26% |
| ⚡ | Simulation | cut_fields_custom_delim |
65.9 ms | 51.9 ms | +27% |
| ⚡ | Simulation | cut_fields_tab |
57.4 ms | 45.5 ms | +26.23% |
| ⚡ | Simulation | cut_bytes |
18.1 ms | 15.5 ms | +16.69% |
| ⚡ | Simulation | sort_key_field[500000] |
1.8 s | 1.5 s | +16.46% |
| ⚡ | Simulation | cut_characters |
25.4 ms | 22.9 ms | +10.88% |
| ⚡ | Simulation | sort_dictionary_order[500000] |
2.1 s | 1.9 s | +8.13% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing lauchimoon:numeric-fix (c1fb65d) with main (fe26d56)
Footnotes
-
54 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
|
@codspeedbot fix this regression on this PR |
I dug into the regression and pushed a fix to this branch. Root cause. This PR's change makes the last-resort comparison in
Fix. I cached the collation decision in a Result. A local CodSpeed simulation run brings the regressed benchmarks back to (or below) the
(Absolute numbers differ from the CI macro runners, but the relative recovery is clear.) The upcoming CodSpeed run on this push will confirm the numbers on CI. Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
|
@codspeedbot fix this regression on this PR |
I looked into this again, and the regression has already been fixed on this branch — no further code change was needed. What's on the branch. The fix that addresses the root cause is already committed ( Verification. I profiled
About the remaining "-5.26%" in the report. That figure comes from comparing this branch's run against a In short: the code-level regression is resolved, verified via the flame graph, and the branch is already up to date. Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
| // If the detected locale is 'C', just do byte-wise comparison. | ||
| // `should_use_locale_collation` caches this decision, avoiding the | ||
| // construction of a fresh `DEFAULT_LOCALE` on every call (hot path in sort). | ||
| if should_use_locale_collation() { | ||
| // Fall back to byte comparison if collator is not available |
There was a problem hiding this comment.
This comment makes more sense if set above left.cmp(right)
| a.line.cmp(b.line) | ||
| #[cfg(feature = "i18n-collator")] | ||
| { | ||
| locale_cmp(a.line, b.line) |
There was a problem hiding this comment.
if the collator says Equal (like 01 vs 0_1), we should fall back to byte compare, as the fast path does at line 2645, otherwise the order isn't deterministic
| } else { | ||
| // If the detected locale is 'C', just do byte-wise comparison. | ||
| // `should_use_locale_collation` caches this decision, avoiding the | ||
| // construction of a fresh `DEFAULT_LOCALE` on every call (hot path in sort). |
There was a problem hiding this comment.
please drop these two lines, the caching is already documented in should_use_locale_collation
| .arg("-n") | ||
| .pipe_in("1 x\n1 ./b\n1 a\n") | ||
| .succeeds() | ||
| .stdout_is("1 a\n1 ./b\n1 x\n"); |
There was a problem hiding this comment.
could you please also test with -r and with two lines the collator treats as equal?
redox is included the unix target_family
Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
Should make test tests/date/date-locale-hour.sh pass
Should make test tests/date/date-locale-hour.sh pass
Should make tests tests/printf/printf.sh and tests/seq/seq-precision.sh pass
traverse() classified the operand from pathname metadata -- is it a directory, does --from match, is it '/' -- and then re-resolved the same pathname twice: once with DirFd::open(path, Follow) for the fchown, and again in safe_dive_into() for the descent. Neither open was checked against the metadata the decisions were made on. An attacker able to rename inside the operand's parent could therefore swap the classified directory for a symlink after the checks, and every fd-anchored mutation below ran inside the symlink target: fchown on the root, chown_at/chmod_at on the descendants. --from and --preserve-root were evaluated against the decoy and never re-checked, so the filter was bypassed rather than merely the traversal. Open the operand once, fstat the descriptor and compare (dev, ino) against that metadata, and fail closed if they differ. The descent reuses that same descriptor, so the pathname is resolved exactly once, and whether to descend at all is now decided from the metadata rather than from fresh is_symlink()/is_dir() lookups. b286558 fixed the descent open (open_subdir with the caller's symlink policy); this is the same fail-closed pattern one level up, at the acquisition of the operand itself. Measured with a rename-exchange harness racing chgrp -R against a symlink to an off-tree directory: before, the off-tree victim was regrouped within 1-4 invocations on every run; after, 1500 invocations left it untouched. GNU chgrp resists the same harness. The static flag matrix (-P/-H/-L, symlink and directory operands, --from, --preserve-root, missing operand) is byte-identical before and after.
walk_dir_with_context checked is_symlink() and is_dir() on the operand pathname and then opened that pathname again with a hardcoded SymlinkBehavior::Follow. An attacker able to rename inside the operand's parent could swap the directory just checked for a symlink in between, and the whole descent ran inside the symlink target -- under -P, which asks for no symlink to be traversed at all. Open with should_follow_symlink instead, the policy the checks were made under: O_NOFOLLOW makes the open fail closed when the operand became a symlink, while -H/-L, where following the operand is what was asked for, are unchanged. This mirrors what the descent already does for entries met inside the tree. Measured with a rename-exchange harness racing chmod -R -P against a symlink to an off-tree directory: before, an off-tree file was re-moded within 1-8 invocations on every run; after, 1500 invocations left it untouched. GNU chmod resists the same harness under -P. Under -H and -L both GNU and uutils follow, as intended. Still path-based, and not addressed here: the operand's own chmod_file call resolves the pathname a third time, so a swap landing in that narrower window re-modes the symlink target itself (not its contents). Closing it needs the operand's mode change to go through the descriptor.
The pre-existing "cannot access" messages in the recursive walk were still hardcoded English. Route them through translate!() with the perms-cannot-access and perms-too-many-symlink-levels keys.
Should make tests tests/head/quote-headers.sh and tests/tail/quote-headers.sh pass
Removed 'tests/date/date-locale-hour' from ignore list.
After creating a hard link, cp -l still ran the final chmod meant for newly copied files on the destination. Since the link shares the source's inode, this changed the mode of the source file: under umask 077 a 755 source became 700, and with -f over an existing file the source took the old destination's mode. GNU cp leaves the linked file untouched. Skip that chmod in link mode.
This reverts commit 781ddfc.
COMMAND and its arguments were read as String, so argument parsing failed when any value was not valid UTF-8. Read them as OsString so they reach the child unchanged, as they do with GNU timeout. DURATION, --signal and --kill-after remain text values. A non-UTF-8 value there still exits 125.
GNU accepts any blanks, or none, on either side of the '=' of a
tagged line, with or without a space before the '(': "MD5(f) = D",
"MD5 (f)= D", "MD5 (f) =D" and "MD5 (f)<TAB>=<TAB>D" are all
checked. uutils only accepted "MD5 (f) = D" and "MD5(f)= D", and
reported the other lines as improperly formatted.
End the file name at the last ')' and skip the blanks around the
'='. If the text before the '(' is all hex digits, keep the exact
separators: "<HEX DIGEST> (a)= b" is an untagged line with a single
space, and must still be read as one.
Update the parser tests that expected such lines to be rejected,
and add tests for these lines, for lines that must stay rejected
as in GNU (a form feed or carriage return next to the '=', two
spaces or a tab before the '('), and for the hex digest case.
updates: - [github.com/streetsidesoftware/cspell-cli: v10.2.0 → v10.3.0](streetsidesoftware/cspell-cli@v10.2.0...v10.3.0)
setup_localization() only ever kept the first utility: the per-thread localizer could not be replaced and the embedded utility resource was a single process-wide OnceLock. Hosts that run several utilities, such as nushell, then printed raw message ids for every utility after the first. Make the thread-local localizer replaceable when a different utility is set up, and cache embedded utility resources per utility.
The help template sets up localization with the invoked name, e.g. dir, vdir or a prefixed binary name. Those have no strings of their own, so switching to them replaced the utility's strings with nothing.
Switching utilities rebuilds bundles, and the on-disk and WASI paths leaked a freshly parsed resource each time. Cache all of them per source instead, so each file is parsed and leaked at most once. Overwrite the localizer only after the new one was built, instead of clearing it first: a failed setup now keeps the previous utility's strings, matching LOCALIZED_UTIL. This also drops the "already initialized" error, which can no longer happen. Replace the thread-based test, which also passed before this change, with one checking that bundles reuse cached resources.
Add UCommand::ignore_sigxfsz, which ignores SIGXFSZ in the child's pre_exec, so a test can push a utility past an Fsize limit and see the failed write. test_dd's two Fsize tests did this with a guard that changed SIGXFSZ for the whole test process. Under cargo test they run in parallel in one binary, and one test restoring the default could get the other's dd killed before it printed its stats. Use the new method instead.
Error construction is infrequently executed compared with the normal success paths. Mark these functions with `#[cold]` so the compiler can treat them as unlikely code paths and keep cold code separate from hot code. Use `#[inline(never)]` to prevent error-handling code from being inlined into their callers, avoiding unnecessary code-size growth and keeping the normal execution paths smaller.
With --to=<scale>, a "%.0f" precision took the div_round path, which keeps one fractional digit below the next base and then rounds that to nearest, so 1234567 printed 1M instead of GNU's 2M under the default from-zero rounding. An explicitly given precision now rounds the scaled value with the selected method.
tee duplicates its input through two pipes on Linux. When splicing from one of them to an output failed part-way, for example with ENOSPC or EFBIG, the bytes the output did not take stayed in the pipe, and the remaining outputs got them in front of their next chunk. Replace the pipe with an empty one after a failed output. The old pipe is closed first, so that this also works at the open file limit. A new pipe gets the larger size only if the 2nd pipe got it at the start, so that the 2nd is never smaller than the 1st. If no new pipe can be made, tee fails with the error.
date implements get_clock_resolution via libc::clock_getres (rustix's excludes WASI) and set_system_datetime for WASI, the latter reporting "not supported" since the sandbox has no wall-clock-set syscall.
The only thing keeping it off them was the stat time fields, which they expose as timespecs (st_atim and so on). Read those there, and gate safe_traversal and its users on Redox alone, so chmod, du, install and rm walk trees through descriptors there too. The path-based chmod helpers that only AIX still used are gone, as are the target-specific safe-traversal features of chmod, chown and install, which already get it through perms. Redox stays excluded: nix has none of the *at calls there.
On aarch64, `cksum --debug` used the ASIMD check for the vmull line, but the vmull path needs PMULL. Check PMULL at runtime, as GNU does. Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
20 participants
Fixes #13319.
When all selectors return
Ordering::Equalincompare_by, the last resort was to use byte-by-byte comparison, ignoring the locale.The change now resorts to use
locale_cmpifi18n-collatoris enabled.