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

Rewrite complexity_test to use (hardcoded) manual time #1757

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

LebedevRI
Copy link
Collaborator

@LebedevRI LebedevRI commented Feb 14, 2024

This test is fundamentally flaky, because it tried to read tea leafs, and is inherently misbehaving in CI environments,
since there are unmitigated sources of noise.

Fixes #272
Fixes #1758

@LebedevRI LebedevRI requested a review from dmah42 February 14, 2024 21:19
@LebedevRI LebedevRI force-pushed the complexity branch 2 times, most recently from b2496c8 to 0806db7 Compare February 14, 2024 21:33
@LebedevRI LebedevRI closed this Feb 14, 2024
@dmah42
Copy link
Member

dmah42 commented Feb 15, 2024

what happened? this seemed like a good idea...

@LebedevRI
Copy link
Collaborator Author

I don't understand what the heck is going on with automatic complexity fitting,
all windows CI failed here.

@LebedevRI LebedevRI reopened this Feb 17, 2024
This test is fundamentally flaky, because it tried to read tea leafs,
and is inherently misbehaving in CI environments,
since there are unmitigated sources of noise.

That being said, the computed Big-O also depends on the `--benchmark_min_time=`

Fixes google#272
@LebedevRI
Copy link
Collaborator Author

OOOOH this is a fun one!

@LebedevRI LebedevRI force-pushed the complexity branch 9 times, most recently from bd48182 to 6107f69 Compare February 17, 2024 03:33
@LebedevRI
Copy link
Collaborator Author

@dmah42 ok, this is more like it.

Looks like on windows, some of these tests still fail,
i guess clock precision is too small.
@@ -96,6 +96,7 @@ BenchmarkReporter::Run CreateRunReport(
} else {
report.real_accumulated_time = results.real_time_used;
}
report.use_real_time_for_initial_big_o = b.use_manual_time();
Copy link
Member

Choose a reason for hiding this comment

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

to check this logic:

if we're using manual time, we'll report manual time as "real" but then use "real" time for bigO.
if we're not using manual time, then we'll report real time as "real" but use "cpu" time for bigO.

maybe this could be a comment somewhere just to track the interaction between manual, real, and cpu times for bigO.

@@ -186,8 +186,19 @@ std::vector<BenchmarkReporter::Run> ComputeBigO(
result_cpu = MinimalLeastSq(n, cpu_time, reports[0].complexity_lambda);
result_real = MinimalLeastSq(n, real_time, reports[0].complexity_lambda);
} else {
result_cpu = MinimalLeastSq(n, cpu_time, reports[0].complexity);
result_real = MinimalLeastSq(n, real_time, result_cpu.complexity);
const BigO* InitialBigO = &reports[0].complexity;
Copy link
Member

Choose a reason for hiding this comment

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

this section needs some comments. maybe there's a way to simplify the logic?

BENCHMARK(BM_Complexity_O1)
->Range(1, 1 << 18)
->UseManualTime()
Copy link
Member

Choose a reason for hiding this comment

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

does this mean complexity only works for manual time? do we have any tests where we use complexity with real time?

@LebedevRI
Copy link
Collaborator Author

@dmah42 we always compute complexity for both cpu and real time,
this only changes which of these two is used to auto-detect the Big-O.

@dmah42 dmah42 merged commit 3d85343 into google:main Feb 19, 2024
80 checks passed
@LebedevRI
Copy link
Collaborator Author

@dmah42 thank you. Looks like there's some residual flakiness in CI left, so i'll see if these nits can be revisited later...

@LebedevRI LebedevRI deleted the complexity branch February 19, 2024 15:34
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.

Complexity calculation does not work for manual timing complexity_test.cc spuriously fails
2 participants