Report the complexity aggregates in the family's time unit - #2294
Report the complexity aggregates in the family's time unit#2294lenamonj wants to merge 6 commits into
Conversation
ComputeBigO left big_o.time_unit at the Run() default, so the BigO row was rendered in nanoseconds whatever unit the family declared; CSVReporter rendered the RMS row from the raw, pre-divided field, so it was smaller by the unit multiplier. Both now go through the family unit and the GetAdjusted*Time() accessors, with a millisecond family in complexity_test.cc that fails on main for both.
| big_o.cpu_accumulated_time = result_cpu.coef; | ||
| big_o.report_big_o = true; | ||
| big_o.complexity = result_cpu.complexity; | ||
| // The coefficient is a time; reporters render it in the family's unit. |
| const std::string& rms_test_name, | ||
| const std::string& big_o, int family_index) { | ||
| const std::string& big_o, int family_index, | ||
| const std::string& time_unit = "ns", |
|
Removed the comment and the defaults; each family passes its unit and CSV pattern explicitly. |
LebedevRI
left a comment
LebedevRI
left a comment
There was a problem hiding this comment.
CSV reporter is deprecated and should not be used.
The CSV reporter is deprecated, so its RMS row is left as it is and the test keeps the pattern it had.
|
Dropped the CSV reporter change and its test pattern; the PR is now the one-line time_unit fix with the JSON check. |
LebedevRI
left a comment
LebedevRI
left a comment
There was a problem hiding this comment.
Ah, hmm, when initially reading the change, somehow i missed that it was also 'fixing' JSON.
I thought it was only making CSV do the same as JSON reporter.
After sleeping on this, i do think this makes sense.
I do have a suggestion though: is there a test that breaks
if you intentionally scale the value of the "bigo" field in JSON reporter
by x1000? There should be one, we should have a test
to ensure that the reported bigo really does have the correct magnitude
as implied by the unit.
Until now, scaling the JSON BigO coefficients by 1000 left every test passing. Under manual time the millisecond family's real coefficient is about 1.0004e-03, so its BigO row now has to match 1\.[0-9]+e-03, and that scaling fails it. The skew comment is corrected: of the family's sizes, 1024 to 65536, only 2048 has bit 11 set.
|
There is now: scaling both coefficients by 1000 in |
3 participants
ComputeBigOnever setbig_o.time_unit, so the BigO row kept theRun()default of nanoseconds and its coefficient was rendered in ns whatever unit the family declared. It now carries the family's unit, as the RMS row already did.The new
BM_Complexity_O_N_msfamily declares milliseconds: on main its BigO row reports"time_unit": "ns". The test fails on main and passes here.