Skip to content

Conversation

@soonum
Copy link
Contributor

@soonum soonum commented Oct 27, 2025

This change is Reviewable

@soonum soonum self-assigned this Oct 27, 2025
@soonum soonum added the ci label Oct 27, 2025
@cla-bot cla-bot bot added the cla-signed label Oct 27, 2025
@soonum soonum added the bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. label Oct 27, 2025
@soonum soonum force-pushed the dt/bench/regression_target branch from 2d5baec to c836091 Compare October 27, 2025 15:32
@soonum soonum added bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. and removed bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. labels Oct 27, 2025
@soonum soonum force-pushed the dt/bench/regression_target branch from c836091 to 2add781 Compare October 27, 2025 16:05
@soonum soonum force-pushed the dt/bench/regression_target branch from 2add781 to 6d97850 Compare October 27, 2025 16:23
@soonum soonum added bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. and removed bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. labels Oct 27, 2025
@soonum
Copy link
Contributor Author

soonum commented Oct 28, 2025

I need to add support for shortint layer in the data extractor.
That being said, execution time for this new regression profile is around 2 hours 15 mins.

@soonum soonum added bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. and removed bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. labels Oct 28, 2025
@soonum soonum marked this pull request as ready for review October 28, 2025 10:34
@soonum soonum requested a review from IceTDrinker October 28, 2025 10:34
Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

A few questions/comments, also, why is it taking 2h+ to get those benchmarks in that case ?

@IceTDrinker reviewed 1 of 1 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @soonum)


ci/regression.toml line 84 at r3 (raw file):

    "swap_claim::no_cmux"
]
target.hlapi-erc20 = ["transfer::whitepaper", "transfer::no_cmux"]

why two variants here ?


ci/perf_regression/perf_regression.py line 634 at r3 (raw file):

        try:
            op_perf = OperationPerformance(op_name, baseline_data, head_branch_value)
        except statistics.StatisticsError as err:

when did you encounter this issue ?


ci/perf_regression/perf_regression.py line 740 at r3 (raw file):

        "<summary><strong>View All Benchmarks</strong></summary>",
        "",
        f"* Minor change threshold: +/- {MINOR_CHANGE_SCALE_FACTOR}x StdDev",

nice idea, do we print by how many std dev the latest value diverges ?

Copy link
Contributor Author

@soonum soonum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @antoniupop and @IceTDrinker)


ci/regression.toml line 84 at r3 (raw file):

Previously, IceTDrinker wrote…

why two variants here ?

Because @antoniupop told me they use both of them in the coprocessor code.


ci/perf_regression/perf_regression.py line 634 at r3 (raw file):

Previously, IceTDrinker wrote…

when did you encounter this issue ?

When there is only one data point in main. Which is unlikely to happen except when we introduce a brand new benchmark (either new operation or a new params set)


ci/perf_regression/perf_regression.py line 740 at r3 (raw file):

Previously, IceTDrinker wrote…

nice idea, do we print by how many std dev the latest value diverges ?

Nope but I can add it easily. Should I do it ?

@IceTDrinker
Copy link
Member

ci/perf_regression/perf_regression.py line 634 at r3 (raw file):

Previously, soonum (David Testé) wrote…

When there is only one data point in main. Which is unlikely to happen except when we introduce a brand new benchmark (either new operation or a new params set)

ok yeah

Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

Thanks a lot ! :)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @soonum)


ci/regression.toml line 84 at r3 (raw file):

Previously, soonum (David Testé) wrote…

Because @antoniupop told me they use both of them in the coprocessor code.

ok surprising which variant is set to be standardized in an ERC ?


ci/perf_regression/perf_regression.py line 740 at r3 (raw file):

Previously, soonum (David Testé) wrote…

Nope but I can add it easily. Should I do it ?

not necessarilyin this PR to keep the focus and avoid having this PR being delayed

Copy link
Contributor Author

@soonum soonum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @soonum)


ci/regression.toml line 84 at r3 (raw file):

Previously, IceTDrinker wrote…

ok surprising which variant is set to be standardized in an ERC ?

No idea. Let's keep the two variants for now and remove the unused if they finally use only one.

@soonum soonum merged commit 3c32b15 into main Oct 29, 2025
136 checks passed
@soonum soonum deleted the dt/bench/regression_target branch October 29, 2025 14:33
Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion


ci/regression.toml line 84 at r3 (raw file):

Previously, soonum (David Testé) wrote…

No idea. Let's keep the two variants for now and remove the unused if they finally use only one.

let's have a backlog issue maybe to avoid forgetting about it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved bench-perfs-cpu Trigger regression benchmark workflow for CPU backend. ci cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants