Fixes #1663 by increasing the kMaxIterations limit.

[andreas-abel]

An alternative could be to get rid of this limit; from the code, it is not clear why it is needed.

[LebedevRI]

My main concern would be that the lines become even longer now.
I suppose i'd be fine with the limit bump, but degradation of the output makes me do a second take.

[dmah42]

increasing the limit vs getting rid of the limit both suffer from the same issue, which is that for slow benchmarks without a minimum time, they just became a lot slower.

[dmah42]

actually i take that back... at 5 x min time we'll also bale out, so maybe this is safe (and it's safe to remove the limit!)

[LebedevRI]

actually i take that back... at 5 x min time we'll also bale out, so maybe this is safe (and it's safe to remove the limit!)

Yeah, i've looked at it's usage, and i'm not really sure if it is not needed or needed.
There's

benchmark/src/benchmark_runner.cc

Lines 330 to 331 in 46d3c84

	// But we do have *some* limits though..
	const IterationCount next_iters = std::min(max_next_iters, kMaxIterations);

which seems to benefit from it.

[dmah42]

actually i take that back... at 5 x min time we'll also bale out, so maybe this is safe (and it's safe to remove the limit!)

Yeah, i've looked at it's usage, and i'm not really sure if it is not needed or needed. There's

benchmark/src/benchmark_runner.cc

Lines 330 to 331 in 46d3c84

	// But we do have *some* limits though..
	const IterationCount next_iters = std::min(max_next_iters, kMaxIterations);

which seems to benefit from it.

yeah, this coupled with the end-of-benchmark check below is what puts the cap on. we could remove both, at which point we'd keep increasing iterations until we hit 5 x mintime. i suspect this would be fine.

i'm also happy with the current patch.

[LebedevRI]

(To repeat myself - limit bump LGTM, printing change NACK)

[andreas-abel]

What's the concern about the line length? The line length is variable already (as it's based on the length of the benchmark name). Why are three additional characters problematic?

[LebedevRI]

What's the concern about the line length? The line length is variable already (as it's based on the length of the benchmark name). Why are three additional characters problematic?

There's always some line width in the terminal/editor/viewer where
the console reporter results are being viewed, even with dynamic word wrap enabled.
The longer the line, the higher the chance the full line won't fit, and user counters
get wrapped to the new line. That's much harder to read, much like code beyond 80'th column.
99.9+% or or of existing benchmarks don't require a trillion iterations,
so optimizing the UX for the 0.01% percent seems suboptimal.

(I vaguely recall having similar discussion in this repo years ago, but i don't have a link)

[LebedevRI]

Really not a fan of reprinting headers...

[andreas-abel]

What do you suggest instead?

According to the statistics in your previous message, reprinting would not happen for 99.9+% of the existing benchmarks.

[dmah42]

@LebedevRI I'm having a hard time think of any alternatives. either the line length gets longer and things misalign (if no printing changes are made), or the line length is always longer (I'm not a fan of this trend either), or the current version where it grows when necessary and the headers get reprinted at that point.

I don't have a strong opinion but I am stuck thinking of anything else to suggest.

[LebedevRI]

@LebedevRI I'm having a hard time think of any alternatives.

The only other unmentioned option is fully buffering the output, but that is even worse.

either the line length gets longer and things misalign (if no printing changes are made)

Note: this is what currently happens when the time fields "overflow".

or the line length is always longer (I'm not a fan of this trend either)

Yup.

or the current version where it grows when necessary and the headers get reprinted at that point.

I don't have a strong opinion but I am stuck thinking of anything else to suggest.

I agree that the solution is elegant. I'm basically unsure if this misalignment is worth the confusion
over the header being reprinted? I don't know, it just seems more confusing to me that way...

What i would certainly suggest is to proceed with the uncontentious change,
and deal with prettiness later.

[dmah42]

@LebedevRI I'm having a hard time think of any alternatives.

The only other unmentioned option is fully buffering the output, but that is even worse.

either the line length gets longer and things misalign (if no printing changes are made)

Note: this is what currently happens when the time fields "overflow".

or the line length is always longer (I'm not a fan of this trend either)

Yup.

or the current version where it grows when necessary and the headers get reprinted at that point.
I don't have a strong opinion but I am stuck thinking of anything else to suggest.

I agree that the solution is elegant. I'm basically unsure if this misalignment is worth the confusion over the header being reprinted? I don't know, it just seems more confusing to me that way...

What i would certainly suggest is to proceed with the uncontentious change, and deal with prettiness later.

by which you mean: change the limit, make no changes to printing and allow any benchmark that goes above the old kMaxIterations to print awkwardly?

i'm ok with that.

[LebedevRI]

by which you mean: change the limit, make no changes to printing and allow any benchmark that goes above the old kMaxIterations to print awkwardly?

i'm ok with that.

Yes.

[andreas-abel]

Would you prefer that I create a new pull request that contains just the uncontentious change, or that I undo the other changes in this pull request?

[LebedevRI]

No preference.

[andreas-abel]

The only other unmentioned option is fully buffering the output, but that is even worse.

Yet another option would be to print it (potentially imprecisely) as a floating point number (something like 1.23e4).

[dmah42]

this is superseded by #1668 right?

[andreas-abel]

Yes, that's correct.