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

5% regression in some Inference benchmarks due to #51149 (Set temporary env whenever Julia is started with --project=@temp) #54850

Closed
Zentrik opened this issue Jun 18, 2024 · 6 comments
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@Zentrik
Copy link
Member

Zentrik commented Jun 18, 2024

I'm confused as to how 583981f has caused this, but it caused a 6% regression in min wall time, allocations and memory for the [["inference", "allinference", "REPL.REPLCompletions.completions"]] benchmark and a 3% regression for the [["inference", "allinference", "Base.init_stdio(::Ptr{Cvoid})"]] benchmark.

I would presume this regression should be fixable given how little the pr changed but I haven't been able to figure out what caused the regression in the first place.

Nanosoldier report: https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_date/2024-06/10/report.md

@Zentrik Zentrik added performance Must go faster regression Regression in behavior compared to a previous version labels Jun 18, 2024
@oscardssmith
Copy link
Member

Are you sure this isn't noise?

@Zentrik
Copy link
Member Author

Zentrik commented Jun 18, 2024

Yes, I can reproduce it reliably and it reproduced on the only other nanosoldier run, see https://tealquaternion.camdvr.org/compare.html?start=2024-06-04&end=2024-06-13&stat=min-wall-time&name=allinferen.

@IanButterworth
Copy link
Member

Sorry if I'm being slow, but what from the reports indicates that commit? There's a huge number of commits between that report and the previous?

@Zentrik
Copy link
Member Author

Zentrik commented Jun 18, 2024

Sorry, I bisected the regression to that commit.

@KristofferC
Copy link
Member

and a 3% regression for the [["inference", "allinference", "Base.init_stdio(::Ptr{Cvoid})"]] benchmark.

This seems like dup of the reason in #53695, #53459 (comment)

@Zentrik
Copy link
Member Author

Zentrik commented Jun 19, 2024

Yes, for both benchmarks it's because it now needs to infer mktempdir in load_path_expand.

@Zentrik Zentrik closed this as completed Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

4 participants