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

Remove unnecessary extra wrapping of valtypes #77

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KevinDCarlson
Copy link
Contributor

There's no difference in functionality between f(::Type{Val{n}}) where n =... and f(::Val{n}) where n = ..., but the latter is both cleaner and the intended usage: you plug in the unique instance of the type Val{n} rather than plugging in the type. I'm not able to find any difference in performance between the two approaches but I wanted to clean this up anyway.

@KevinDCarlson KevinDCarlson requested a review from lukem12345 May 1, 2024 02:42
@epatters
Copy link
Member

epatters commented May 1, 2024

Have you done benchmarks? I'm not sure how it shakes out in this case, but my understanding is that there is sometimes a reason to prefer ::Type{Val{T}} to ::Val{T}, namely that Val{t}, where t is a literal, is a constant available at compile time and can therefore be constant propagated, whereas Val(t) is value that might only be constructed and dispatched at runtime.

Or, at least at one time in the past, this was a distinction that bit me. Perhaps, due to compiler improvements, it is no longer relevant. I would suggest getting advice from someone who actually understands how the Julia compiler works (not me).

@jpfairbanks
Copy link
Member

This PR would make things much more idiomatic, but we do need the benchmarks, since this code is so critical to decapodes performance. A quick and dirty way to check, since you are touching such a large fraction of the codebase is to just run the tests before and after the change. If the tests take longer, then likely this is the culprit.

@KevinDCarlson
Copy link
Contributor Author

This PR would make things much more idiomatic, but we do need the benchmarks, since this code is so critical to decapodes performance. A quick and dirty way to check, since you are touching such a large fraction of the codebase is to just run the tests before and after the change. If the tests take longer, then likely this is the culprit.

Oh I definitely did that, it gets like 1-2% faster IIRC. Also tried some synthetic tests but wasn’t able to show the difference ever affects performance.

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