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

make the constructorof fallback method non-@generated #98

Closed
wants to merge 1 commit into from

Conversation

nsajko
Copy link

@nsajko nsajko commented Nov 7, 2024

I don't see a justification for using @generated. The effects are good without it:

julia> Base.infer_effects(constructorof, Tuple{Type{Int}})
(+c,+e,+n,+t,+s,+m,+u,+o,+r)

Updates #21

I don't see a justification for using `@generated`. The effects are
good without it:

```julia-repl
julia> Base.infer_effects(constructorof, Tuple{Type{Int}})
(+c,+e,+n,+t,+s,+m,+u,+o,+r)
```

Updates JuliaObjects#21
@rafaqz
Copy link
Member

rafaqz commented Nov 7, 2024

Yes, this is from before the effects system existed. (oh you mean not even using assume_effects!)

One justification is simply caution, as this function is used by a quarter of the ecosystem, and sometimes in hot inner loops.

We know the generated function will always work with every new Julia version. We hope the effects system will too, but we have no guarantee.

What is the problem the generated function is causing?

@nsajko
Copy link
Author

nsajko commented Nov 7, 2024

Ah, I just realized you still support Julia v1. I guess it's too early to merge this, then?

What is the problem the generated function is causing?

There's no specific problem, it's just good to avoid them when possible. Rule of least power comes to mind. Generated functions work on a lower level than usual functions, bypassing many parts of Julia, making them less safe.

I think getting rid of @generated could also bring compiler latency improvements, but I didn't look into it at all.

@rafaqz
Copy link
Member

rafaqz commented Nov 7, 2024

I don't know if that's a good enough reason? If there is a practical use being blocked then sure. But currently @generated is reliable, and if you look through issue history removing generated functions has caused problems in the past.

I think getting rid of @generated could also bring compiler latency improvements, but I didn't look into it at all.

I would love to see a benchmark of this. In some cases well written generated functions may be faster to compile than the alternatives. But in this PR I guess there is barely any difference?

@jw3126
Copy link
Member

jw3126 commented Nov 7, 2024

I agree with @rafaqz . There needs to be some kind of measurable benefit for getting rid of @generated here.

If there is one, happy to have an if VERSION branch without the @generated.

@nsajko nsajko closed this Nov 7, 2024
@nsajko nsajko deleted the no_generated branch November 7, 2024 16:21
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