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

Consolidate implementation of _process_totals #523

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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 (88470fc) with main (d9752db)

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

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?

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.

2 participants