Skip to content
This repository was archived by the owner on May 5, 2025. It is now read-only.

Consolidate implementation of _process_totals #523

Merged
merged 2 commits into from
Feb 24, 2025

Conversation

Swatinem
Copy link
Contributor

In particular, this unifies the duplicated implementations of these methods across Filtered/ReportFile.

The new method also spells out a "very concise, but also unreadable" implementation that sums up the cyclomatic complexity (something I would like to remove completely eventually). This is now more readable and understandable, but more importantly, it only iterates over the lines once, instead of twice as the previous implementation did. Along with avoiding a bunch of intermediate list allocations that the FilteredReportFile implementation did, this should yield quite a good speedup.


I get the following benchmarks results now locally (as sadly, benchmarks are not yet fully working in CI):

                                         Benchmark Results                                          
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━┓
┃                                       Benchmark ┃   Time (best) ┃ Rel. StdDev ┃ Run time ┃ Iters ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━┩
│                     test_report_parsing[Report] │       7,184ns │        2.9% │    3.02s │ 5,396 │
│             test_report_parsing[ReadOnlyReport] │       8,229ns │        3.1% │    3.01s │ 5,041 │
│        test_report_parsing[Rust ReadOnlyReport] │       8,815ns │        1.4% │    2.94s │ 4,828 │
│             test_report_parsing[EditableReport] │     195,443ns │        0.2% │    2.96s │ 1,005 │
│                 test_report_iterate_all[Report] │  47,338,292ns │        0.3% │    2.90s │    61 │
│         test_report_iterate_all[ReadOnlyReport] │  46,308,167ns │        1.7% │    2.95s │    63 │
│    test_report_iterate_all[Rust ReadOnlyReport] │  47,148,833ns │        1.3% │    2.94s │    62 │
│         test_report_iterate_all[EditableReport] │  45,321,666ns │        0.9% │    2.96s │    65 │
│              test_report_process_totals[Report] │  58,634,083ns │        0.7% │    2.97s │    50 │
│      test_report_process_totals[ReadOnlyReport] │  57,668,083ns │        0.4% │    2.90s │    50 │
│ test_report_process_totals[Rust ReadOnlyReport] │  58,458,584ns │        1.6% │    3.00s │    50 │
│      test_report_process_totals[EditableReport] │  55,609,125ns │        1.3% │    2.92s │    52 │
│                   test_report_filtering[Report] │ 178,210,958ns │        0.8% │    2.87s │    16 │
│           test_report_filtering[ReadOnlyReport] │ 184,516,500ns │        1.1% │    2.99s │    16 │
│      test_report_filtering[Rust ReadOnlyReport] │  74,746,791ns │        0.4% │    2.94s │    39 │
│           test_report_filtering[EditableReport] │ 170,324,791ns │        0.7% │    2.93s │    17 │
│                   test_report_serialize[Report] │       2,131ns │        2.6% │    2.97s │ 9,869 │
│           test_report_serialize[EditableReport] │   2,887,927ns │        1.5% │    2.98s │   256 │
│                       test_report_merge[Report] │ 239,321,583ns │        0.4% │    2.89s │    12 │
│               test_report_merge[EditableReport] │ 249,947,583ns │        0.2% │    2.76s │    11 │
│                test_report_carryforward[Report] │   2,933,698ns │        0.3% │    2.93s │   248 │
│        test_report_carryforward[EditableReport] │   2,941,503ns │        1.1% │    2.90s │   244 │
└─────────────────────────────────────────────────┴───────────────┴─────────────┴──────────┴───────┘

Comparing this with the numbers in #512, we can see that the process_totals benchmark got a speedup of 2x (-50ms), and the filtering benchmark as well got sped up by -50ms.

In particular, this unifies the duplicated implementations of these methods across `Filtered/ReportFile`.

The new method also spells out a "very concise, but also unreadable" implementation that sums up the cyclomatic complexity (something I would like to remove completely eventually).
This is now more readable and understandable, but more importantly, it only iterates over the `lines` *once*, instead of twice as the previous implementation did.
Along with avoiding a bunch of intermediate list allocations that the `FilteredReportFile` implementation did, this should yield quite a good speedup.
@Swatinem Swatinem self-assigned this Feb 21, 2025
@Swatinem Swatinem requested a review from a team February 21, 2025 13:30
Copy link

codspeed-hq bot commented Feb 21, 2025

CodSpeed Performance Report

Merging #523 will create unknown performance changes

Comparing swatinem/consolidate-totals (0b4d0e3) with main (e6906af)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.\

Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

LGTM, left some questions

complexity_total = 0

for line in lines:
match line_type(line.coverage):
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] line_type and line.type (below) are confusing... Maybe we can rename line_type( something like line_coverage_type or line_coverage

If they are the same thing, why can't we match the block below in this match?

if line.type == "b":
    branches += 1
elif line.type == "m":
    methods += 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the imports for the line_type and LineType above to make the distinction a little clearer.

elif line.type == "m":
methods += 1

if line.messages:
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A completely unused thing :-D Otherwise I don’t think anyone really knows.

@Swatinem Swatinem enabled auto-merge February 24, 2025 08:17
@Swatinem Swatinem added this pull request to the merge queue Feb 24, 2025
Merged via the queue into main with commit c3a2f78 Feb 24, 2025
8 checks passed
@Swatinem Swatinem deleted the swatinem/consolidate-totals branch February 24, 2025 08:22
@JerrySentry JerrySentry restored the swatinem/consolidate-totals branch March 11, 2025 21:00
JerrySentry added a commit that referenced this pull request Mar 11, 2025
Copy link

overwatch-beta bot commented Mar 11, 2025

✅ Sentry found no issues in your recent changes ✅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants