Skip to content

tsort: add sequential access hint to applicable platforms for performance - #14415

Open
anastygnome wants to merge 1 commit into
uutils:mainfrom
anastygnome:tsort2
Open

tsort: add sequential access hint to applicable platforms for performance #14415
anastygnome wants to merge 1 commit into
uutils:mainfrom
anastygnome:tsort2

Conversation

@anastygnome

Copy link
Copy Markdown
Contributor

On unix plateforms, we advise the OS of the sequential access to the graph file.
There's no reason not to do it on windows too

The change to the code is minimal touse File::Options to open the file and there's virtually no cost for this
NOTE : we now open the input file read-only explicitely, but that's arguably better.

waiting for #14384 to be merged. Already includes it.

@anastygnome
anastygnome force-pushed the tsort2 branch 4 times, most recently from 858de65 to aacc8c1 Compare September 6, 2026 16:42
@sylvestre

Copy link
Copy Markdown
Contributor

you did much more than the title suggests, no ? :)

@anastygnome
anastygnome force-pushed the tsort2 branch 3 times, most recently from b36dd8b to 7c56e99 Compare September 6, 2026 17:00
@anastygnome

Copy link
Copy Markdown
Contributor Author

@sylvestre please merge #14384 it's an important security fix, I just included it here to do the tests as I don't have access to a windows machine ^^'

@oech3

oech3 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Is it able to apply this also for target_os = "cygwin"?

@anastygnome

Copy link
Copy Markdown
Contributor Author

Is it able to apply this also for target_os = "cygwin"?

It should, it's now exposed on cygwin by rustix, and it's cfg unix, so it should work, I added it

@anastygnome
anastygnome force-pushed the tsort2 branch 3 times, most recently from 1caa1c6 to 0790d8b Compare September 6, 2026 17:28
@anastygnome anastygnome changed the title tsort: add sequential access hint to windows for performance tsort: add sequential access hint to applicable platforms for performance Sep 6, 2026
@anastygnome
anastygnome marked this pull request as ready for review September 6, 2026 17:46
@anastygnome

Copy link
Copy Markdown
Contributor Author

@sylvestre ready to merge ;)

@codspeed-hq

codspeed-hq Bot commented Sep 6, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 367 untouched benchmarks
⏩ 50 skipped benchmarks1


Comparing anastygnome:tsort2 (ed3682c) with main (5f9e12e)

Open in CodSpeed

Footnotes

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

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/cut/bounded-memory (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/retry (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/symlink (passes in this run but fails in the 'main' branch)

Comment thread src/uu/tsort/src/tsort.rs
target_os = "solaris",
target_os = "vita",
))
))]

@oech3 oech3 Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Too complex and most targets are out of scope of uutils. Would you list supported targets instead?

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.

This is a not clause though. I did this to have extensibility, and not having to align with rustix whenever we'll add a target

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.

One day rustc will support is_function_accsssible and this hack won't be needed

@oech3

oech3 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

It might better to add a open wrapper doing fadvise on many unix, FILE_FLAG_SEQUENTIAL_SCAN on Windows, and no-op on others to uucore::io

@anastygnome

anastygnome commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

It might better to add a open wrapper doing fadvise on many unix, FILE_FLAG_SEQUENTIAL_SCAN on Windows, and no-op on others to uucore::io

Cannot be done cleanly, sadly, as on winfows it must be done before opening, and on Linux afterwards, it will still be convoluted, and we still have to manage the isdir case on windows which is hard to get right. It's not that hard to read for now with a comment

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.

3 participants