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

update StiffDDE benchmarks #235

Merged
merged 1 commit into from
May 10, 2021
Merged

update StiffDDE benchmarks #235

merged 1 commit into from
May 10, 2021

Conversation

ChrisRackauckas
Copy link
Member

No description provided.

@ChrisRackauckas
Copy link
Member Author

These ones show the same regression @devmotion @YingboMa @chriselrod

@devmotion
Copy link
Member

Is it possible to view the regressions somewhere? I checked the other PR but it was not clear to me how to figure out what the differences are and what package versions are used by the existing benchmarks (it seems there was no Manifest.toml?).

@ChrisRackauckas
Copy link
Member Author

It's going to be hard because the old benchmarks were just "it's on v1.4 sometime" 🤷 . This now has a manifest and will have a history of all manifests, but yes, tracking down what it is might be hard. It might just be easier to simply dig in and find a 30%-50% speedup, which is around the amount that regressed.

@devmotion
Copy link
Member

These are the changes since 5.24.0 (which was set in the Project.toml): SciML/DelayDiffEq.jl@v5.24.0...v5.30.1

It contains mainly:

@devmotion
Copy link
Member

I wonder if NonlinearSolve causes the regressions?

@chriselrod
Copy link
Contributor

chriselrod commented May 10, 2021

Roots.jl's performance is pretty bad, so I would've expected NonlinearSolve to help.

@ChrisRackauckas
Copy link
Member Author

It could just be some broadcast regressions, if broadcast is used in the repo, that needs to get replaced to FastBroadcast.jl

@chriselrod
Copy link
Contributor

Unless it's using @. somewhere instead of @.., it should already be updated if using an updated DiffEqBase:
https://github.com/SciML/DelayDiffEq.jl/blob/17c4cddabf043afec9346500a75c03a40af68049/src/DelayDiffEq.jl#L17

But it could be that it didn't fix the problem in DelayDiffEq (for both NonStiff and Stiff), in which case we'd need to look into it and fix it.

All these SciMLBenchmarks being run now are the first set of real performance tests for FastBroadcast.jl, so not unlikely that we run into something to fix.

I'll change how safeivdep works to make it possible for others to whitelist their own types, so that the various array types used in the SciML & DiffEq ecosystems can opt in.
I'm also fine with making it just always ivdep like the old one did, apparently without causing problems.

@ChrisRackauckas
Copy link
Member Author

I'm just going to merge the DelayDiffEq ones, but we should keep this in mind. We can track it in SciML/DelayDiffEq.jl#213

@ChrisRackauckas ChrisRackauckas merged commit 548bef0 into master May 10, 2021
@ChrisRackauckas ChrisRackauckas deleted the stiffdde branch May 10, 2021 16:27
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.

3 participants