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

Performance regressions in BaseBenchmarks due to #54647 (Cleanup MemoryRef) #54811

Open
Zentrik opened this issue Jun 14, 2024 · 4 comments
Open
Assignees
Labels
arrays [a, r, r, a, y, s] performance Must go faster regression Regression in behavior compared to a previous version
Milestone

Comments

@Zentrik
Copy link
Member

Zentrik commented Jun 14, 2024

Both [["array", "index", ("sumeach_view", "BitMatrix")]] and [["array", "index", ("sumlinear_view", "BitMatrix")]] have a ~50% regression in min wall time.

[["union", "array", ("skipmissing", "filter", "Union{Missing, Int8}", 1)]], [["union", "array", ("skipmissing", "filter", "Union{Missing, Int64}", 1)]], [["union", "array", ("skipmissing", "filter", "Bool", 0)]] and [["union", "array", ("skipmissing", "filter", "Int64", 0)]] have a ~20% regression in min wall time.

All were bisected to fa038d9.
Here's the nanosoldier report, https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_date/2024-06/10/report.md.
The BitMatrix regression doesn't show up in the report due to a high tolerance, the data is at https://tealquaternion.camdvr.org/compare.html?start=b8e714dfa3d81edd56ccda853322b6e42d94cf60&end=77c28ab286f48afe2512e2ae7f7310b87ca3345e&stat=min-wall-time&name=BitM instead.

@oscardssmith
Copy link
Member

that's rather surprising... It was basically just a rename?

@oscardssmith oscardssmith added performance Must go faster regression Regression in behavior compared to a previous version arrays [a, r, r, a, y, s] labels Jun 14, 2024
@oscardssmith oscardssmith self-assigned this Jun 14, 2024
@Zentrik
Copy link
Member Author

Zentrik commented Jun 14, 2024

c8ae7a0 fixed the regression, so seems like it definitely was due to fa038d9.

@vtjnash
Copy link
Member

vtjnash commented Jan 17, 2025

I think #54681 was merged to revert it

@vtjnash vtjnash closed this as completed Jan 17, 2025
@Zentrik
Copy link
Member Author

Zentrik commented Jan 17, 2025

No, I don't think so? If you look at the Nanosoldier report you'll see the commit range b8e714d...77c28ab includes #54681 and yet we saw a regression.

Though a lot of these benchmarks did improve with 17857ca and something in 58b239c...6e5e87b, looks like [["union", "array", ("skipmissing", "filter", "Union{Missing, Int64}", 1)]] still hasn't improved.

Image

https://tealquaternion.camdvr.org/graphs.html?start=2024-06-04&benchmark=union.array.%28skipmissing%2C+filter%2C+Union%7BMissing%2C+Int64%7D%2C+1%29&stat=min-wall-time&kind=raw

@Zentrik Zentrik reopened this Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants