Skip to content

Conversation

@mattsu2020
Copy link
Contributor

Modifications were made to pass the GNU coreutils tests.

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

ctrlc = { workspace = true }
fnv = { workspace = true }
itertools = { workspace = true }
libc = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

please use nix here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

rng().sample(rand::distr::StandardUniform)
}

fn salt_from_random_source(path: &Path) -> UResult<[u8; 16]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comment to the function and the magic numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add


let mut count = 0usize;
for fd in 0..limit {
if unsafe { libc::fcntl(fd as i32, libc::F_GETFD) } != -1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid the unsafe here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove unsafe

@sylvestre
Copy link
Contributor

some jobs are failing and please add tests

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 26, 2025

CodSpeed Performance Report

Merging #9849 will improve performance by 16.19%

Comparing mattsu2020:sort/sort-merge-fdlimit.sh (9e9b311) with main (44e0bba)

Summary

⚡ 10 improvements
✅ 129 untouched
⏩ 37 skipped1

Benchmarks breakdown

Benchmark BASE HEAD Efficiency
sort_numeric_utf8_locale 22.8 ms 20.2 ms +12.91%
sort_accented_data[500000] 351.8 ms 338.1 ms +4.06%
sort_ascii_c_locale 20.3 ms 18.5 ms +9.65%
sort_ascii_only[500000] 343 ms 329.4 ms +4.11%
sort_reverse_locale[500000] 350.9 ms 336.1 ms +4.41%
sort_case_insensitive[500000] 265.9 ms 255.6 ms +4.02%
sort_case_sensitive[500000] 163.1 ms 151.4 ms +7.77%
sort_ascii_utf8_locale 20.1 ms 19.2 ms +4.8%
sort_long_line[160000] 1.7 ms 1.4 ms +16.19%
sort_mixed_data[500000] 317.5 ms 303.9 ms +4.5%

Footnotes

  1. 37 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.

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

2 similar comments
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

@sylvestre
Copy link
Contributor

please have a look to the perf regression, 9% is a bit too much

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

@mattsu2020 mattsu2020 force-pushed the sort/sort-merge-fdlimit.sh branch from a7a24fa to cafb3ca Compare January 5, 2026 10:55
@github-actions
Copy link

github-actions bot commented Jan 5, 2026

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

@mattsu2020 mattsu2020 marked this pull request as draft January 5, 2026 23:27
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

Update current_open_fd_count to use unsafe libc::fcntl instead of nix::fcntl,
ensuring -1 return for invalid file descriptors, which is more reliable for
counting open fds and avoids potential errors. Added GETFD to spell-checker ignore list.
Make the ctrlc dependency and signal handler optional for Redox OS, which lacks signal support. Added cfg attributes to skip signal handling on Redox, preventing compilation errors. Also added clippy allow attributes for FreeBSD in fs.rs.
Exclude redox, fuchsia, haiku, solaris, and illumos from Unix-specific file descriptor soft limit functionality in get_rlimit and fd_soft_limit, returning None on unsupported platforms to avoid compilation or runtime issues.
- Introduce Vec<Range<usize>> token_buffer field to ChunkContents, RecycledChunk, and related impls
- Modify parse_lines to reuse and clear token_buffer instead of creating a new vec, improving performance
- Update Chunk recycling to include token_buffer in take and init operations
- Pass token_buffer mutably to Line::create for token range storage during sorting
- Bumped versions of multiple crates (e.g., aho-corasick 1.1.3→1.1.4, anstream 0.6.19→0.6.21, windows-sys 0.59.0→0.61.2) for bug fixes, security patches, and performance improvements.
- Removed libc dependency from a package and renamed wasi to wasip2 as part of upstream changes.
@mattsu2020 mattsu2020 force-pushed the sort/sort-merge-fdlimit.sh branch from d4c4389 to 1a1eabd Compare January 6, 2026 04:35
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

- Updated jiff from 0.2.17 to 0.2.18
- Updated jiff-static from 0.2.17 to 0.2.18
- Updated libc from 0.2.178 to 0.2.179
- Updated proc-macro2 from 1.0.104 to 1.0.105
- Updated quote from 1.0.42 to 1.0.43
- Updated hashbrown from 0.15.4 to 0.15.5
- Updated syn from 2.0.111 to 2.0.113
- Updated zmij from 1.0.2 to 1.0.12

These updates bring bug fixes, security patches, and performance improvements.
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

Remove stale package repository SQLite file and force update before installing packages in FreeBSD CI workflows. This prevents installation failures due to outdated package databases, improving reliability in automated builds.
Updated the prepare steps in both FreeBSD CI jobs to write the repository configuration to /usr/local/etc/pkg/repos/FreeBSD.conf before updating and installing packages. This ensures the correct pkg repository is used, preventing potential update failures or outdated package sources.
Add mkdir -p /usr/local/etc/pkg/repos to prepare steps in both jobs to prevent configuration failures if the directory is missing.
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

GNU test failed: tests/shuf/shuf-reservoir. tests/shuf/shuf-reservoir is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/sort/sort-stale-thread-mem. tests/sort/sort-stale-thread-mem is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!
Congrats! The gnu test tests/tail/follow-name is no longer failing!

mattsu2020 and others added 3 commits January 6, 2026 15:19
Refactor parse_lines to conditionally calculate line_count based on whether
selections, num_infos, floats, or numeric mode require reservations,
avoiding unnecessary computation when line_count is not used.
Always compute line count and reserve vector capacities, removing the conditional 'needs_line_count' check for cleaner code and consistent behavior.
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!

Add `line_breaks` field to `ChunkContents` and `RecycledChunk` structs to store separator indices.
Modify `parse_lines` function to collect all line break positions in a single pass, avoiding redundant iterations over input data.
This improves performance by reducing the number of times the input is scanned for separators.
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

GNU test failed: tests/shuf/shuf-reservoir. tests/shuf/shuf-reservoir is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/sort/sort-stale-thread-mem. tests/sort/sort-stale-thread-mem is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!
Congrats! The gnu test tests/tty/tty-eof is no longer failing!

…ry efficiency

Replace the `line_breaks` vector, which stored all line separator indices, with a `line_count_hint` usize field. This change estimates the number of lines using the hint (or a default calculation) instead of pre-computing and storing all break positions, reducing memory allocations and improving performance in the sort utility's chunk parsing logic. The hint defaults to a calculated estimate based on buffer size if not provided.
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/shuf/shuf-reservoir is no longer failing!
Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!
Congrats! The gnu test tests/sort/sort-stale-thread-mem is no longer failing!

…nd unused features

- Compute exact line count for chunks <= 64KB or when hint is 0 to improve accuracy
- Skip tokenization and data extraction in Line::create when not required by settings, enhancing performance for simple sorts or small inputs
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/shuf/shuf-reservoir is no longer failing!
Congrats! The gnu test tests/sort/sort-merge-fdlimit is no longer failing!
Congrats! The gnu test tests/sort/sort-stale-thread-mem is no longer failing!

@mattsu2020 mattsu2020 marked this pull request as ready for review January 6, 2026 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants