Skip to content

Increase the kMaxIterations limit - #1668

Merged
dmah42 merged 7 commits into
google:mainfrom
andreas-abel:time_simple
Oct 17, 2023
Merged

Increase the kMaxIterations limit#1668
dmah42 merged 7 commits into
google:mainfrom
andreas-abel:time_simple

Conversation

@andreas-abel

@andreas-abel andreas-abel commented Sep 20, 2023

Copy link
Copy Markdown
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).

@andreas-abel
Increase the kMaxIterations limit
970b757 
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
View reviewed changes

@LebedevRI LebedevRI left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LG, thank you!

@dmah42 dmah42 mentioned this pull request Sep 26, 2023
Closed
@dmah42

dmah42 commented Sep 26, 2023

Copy link
Copy Markdown
Member

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

dmah42 commented Oct 17, 2023

Copy link
Copy Markdown
Member

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

@andreas-abel

andreas-abel commented Oct 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

I think it should be fixed there.

@dmah42

dmah42 commented Oct 17, 2023

Copy link
Copy Markdown
Member

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

andreas-abel commented Oct 17, 2023

Copy link
Copy Markdown
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

dmah42 commented Oct 17, 2023

Copy link
Copy Markdown
Member

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

@LebedevRI

LebedevRI commented Oct 17, 2023

Copy link
Copy Markdown
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
@dmah42

dmah42 commented Oct 17, 2023

Copy link
Copy Markdown
Member

thank you!

@LebedevRI

LebedevRI commented Oct 17, 2023

Copy link
Copy Markdown
Collaborator

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmah42 dmah42 dmah42 approved these changes
@LebedevRI LebedevRI LebedevRI left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] --benchmark_min_time flag not working as intended

3 participants

@andreas-abel @dmah42 @LebedevRI