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

Increase the kMaxIterations limit #1668

Merged
merged 7 commits into from
Oct 17, 2023
Merged

Conversation

andreas-abel
Copy link
Contributor

This pull request fixes #1663. Note that as a result of this change, the columns in the console output can become misaligned if the actual iteration count is too high. This will be dealt with separately (see also #1664).

This fixes google#1663. Note that as a result of this change, the columns in the console output can become misaligned if the actual iteration count is too high. This will be dealt with in a separate commit.
LebedevRI
LebedevRI previously approved these changes Sep 20, 2023
Copy link
Collaborator

@LebedevRI LebedevRI left a comment

Choose a reason for hiding this comment

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

LG, thank you!

@dmah42
Copy link
Member

dmah42 commented Sep 26, 2023

Assertion failed: iters > i.iters && "if we did more iterations than we want to do the next time, " "then we should have accepted the current iteration run.", file src/benchmark_runner.cc, line 444

in the bazel windows run.

@dmah42
Copy link
Member

dmah42 commented Oct 17, 2023

approved, but there's more lint errors i think.

@andreas-abel
Copy link
Contributor Author

andreas-abel commented Oct 17, 2023

It looks like the other lint error actually comes from this PR: #1681

I think it should be fixed there.

@dmah42
Copy link
Member

dmah42 commented Oct 17, 2023

It looks like the other lint error actually comes from this PR: #1681

I think it should be fixed there.

that's been merged and didn't have a lint issue.. maybe try updating this branch to head?

@andreas-abel
Copy link
Contributor Author

Actually, it looks like the other one does have this issue, even though it was merged: https://github.com/google/benchmark/actions/runs/6535161131/job/17744142279

@dmah42
Copy link
Member

dmah42 commented Oct 17, 2023

oh woops. i thought i had lint set as a required CI. i'll fix that separately then.

@LebedevRI
Copy link
Collaborator

oh woops. i thought i had lint set as a required CI. i'll fix that separately then.

Oops...

@dmah42 dmah42 merged commit f30c99a into google:main Oct 17, 2023
54 of 60 checks passed
@dmah42
Copy link
Member

dmah42 commented Oct 17, 2023

thank you!

@LebedevRI
Copy link
Collaborator

thank you!

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.

[BUG] --benchmark_min_time flag not working as intended
3 participants