Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ci-bench more deterministic #1444

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Conversation

aochagavia
Copy link
Contributor

@aochagavia aochagavia commented Sep 4, 2023

  • Switch to unbuffered stdio
  • Use FxHasher where possible

Note: the CI failure is expected because the benchmarking code has changed between main and this branch (and because main is noisy).

@aochagavia
Copy link
Contributor Author

I have another improvement in mind, which is to automatically output the cachegrind diff for the benchmarks that have significant instruction differences. I'll leave that for tomorrow, though

@@ -116,8 +117,8 @@ fn main() -> anyhow::Result<()> {
.get(index as usize)
.ok_or(anyhow::anyhow!("Benchmark not found: {index}"))?;

let mut stdin = io::stdin().lock();
let mut stdout = io::stdout().lock();
let mut stdin = util::UnbufferedStdinReader;
Copy link
Member

Choose a reason for hiding this comment

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

if i've understood this correctly, it would be equivalent to do let mut stdin = unsafe { File::from_raw_fd(0) };

Copy link
Contributor Author

@aochagavia aochagavia Sep 5, 2023

Choose a reason for hiding this comment

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

I like that! The only drawback I see is that each File will be closed upon drop, meaning that stdin and stdout will get closed too. I think that's not a real problem, though, because if we ever want to use stdio elsewhere, we'll want to do so in an unbuffered way (reusing the files).

(Just pushed the changes use File)

Copy link
Member

Choose a reason for hiding this comment

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

Could do a mem::forget() for the files, I suppose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I'll add that for ultimate correctness

@aochagavia aochagavia force-pushed the ci-bench-determinism branch 2 times, most recently from 1691a73 to 78a8b31 Compare September 5, 2023 07:38
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #1444 (d440dbc) into main (95780ab) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1444   +/-   ##
=======================================
  Coverage   96.49%   96.49%           
=======================================
  Files          71       71           
  Lines       15160    15160           
=======================================
  Hits        14628    14628           
  Misses        532      532           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aochagavia
Copy link
Contributor Author

aochagavia commented Sep 5, 2023

The failing CI jobs seem transient

* Switch to unbuffered stdio
* Use FxHasher where possible
auto-merge was automatically disabled September 5, 2023 09:26

Head branch was pushed to by a user without write access

@ctz ctz added this pull request to the merge queue Sep 5, 2023
Merged via the queue into rustls:main with commit e35a1bc Sep 5, 2023
19 of 22 checks passed
@aochagavia aochagavia deleted the ci-bench-determinism branch September 5, 2023 09:47
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.

None yet

3 participants