-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
Manually union split chunksize calculation #1536
Conversation
Compile time is not improved, but having everything be non-dynamic in its behavior is nice for other reasons... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth refactoring things a bit.
Ideally, the choice of chunk size would be made at a point where types converge again, so that you don't ever actually have to deal with these unions or worry about max_methods
.
elseif chunk_size > 4 | ||
cs = Val{4}() | ||
remake(alg,chunk_size=cs) | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding branches for 2
and 3
? It'd cost some compile time, but I'm worried about possible runtime regressions for small problems.
This comment is also conditional on max_methods
.
Currently, you're getting a union of 3 return types.
That breaks when we set max_methods<3
(e.g. =1
).
My proposal (5 return types) breaks currently. Hence, the above comment on performing this split at a point of return type convergence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break PreallocationTools.jl because it just defaults to pickchunksize
https://github.com/SciML/PreallocationTools.jl/blob/master/src/PreallocationTools.jl#L26
It can handle resizing to smaller chunksizes though. |
Did you run the script on the same computer? Looks like that the inference time regressed. |
Yeah.. 1,2,3,4,8 would be best. 1 or 5 are the best choices, 3 is a bad one 😅 |
Dear god the compiler team would hate me. |
Yeah, I mentioned that above.
I don't know if there's also a runtime hit too. So it's hard to tell if this is a real PR or something mostly for testing. |
I think 4 and 8 will be good, especially with JuliaDiff/ForwardDiff.jl@d033d2a |
Have you tried to only split 4 and 1? |
Before:
After:
That's without the static array handling branch.