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 gcd/lcm effect-free by using LazyString #44935

Merged
merged 5 commits into from
Apr 12, 2022

Conversation

tkf
Copy link
Member

@tkf tkf commented Apr 11, 2022

Before:

julia> Base.infer_effects(gcd, Tuple{Int,Int})
(?c,?e,?n,?t)′

After:

julia> Base.infer_effects(gcd, Tuple{Int,Int})
(?c,+e,?n,?t)

When playing with auto-parallelization (#43910, https://github.com/JuliaFolds/ParallelMagics.jl), I wanted to use gcd in a demo but it didn't work. I think it's a relatively simple change and also in general it's nice to have. But it's a relatively low-stakes "fix" for me and so I'm OK to not do this if there are some potential problems.

@oscardssmith
Copy link
Member

Does this increase the amount of codegen for errors? Should we start reporting the value for all our errors?

@tkf
Copy link
Member Author

tkf commented Apr 11, 2022

Does this increase the amount of codegen for errors?

I think the main difference is a construction of a string vs a construction of a tuple. I'd guess they are not very different and also negligible. But I don't know exactly about it and also how this part of code is used.

I forgot to post the OP initially but, as I just mentioned above, it's really just a nice-to-have change for me.

Comment on lines -154 to +155
throw(OverflowError(Base.invokelatest(string, x, " ", op, " ", y, " overflowed for type ", typeof(x)))))
throw(OverflowError(LazyString(x, " ", op, " ", y, " overflowed for type ", typeof(x)))))
Copy link
Member Author

@tkf tkf Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This invokelatest was added in #30566 for reducing the latency. But, since with LazyString we also avoid the compilation of print of each interpolated values, I'd guess it has a similar effect?

Though I wonder if we need @nospecialize in the constructor?

LazyString(args...) = new(args)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, LazyString should hide the inference cost for printing values at a time when we compile the parent code containing LazyString constructor (and the deferred printing will essentially be dynamically dispatched as like invokelatest).

We generally don't need @nospecialize annotation when a generic function has only a few # of methods and the method body is very simple. But it won't hurt anything (at least at this moment) if we add @nospecialize for this constructor anyway.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to use lazy"..." syntax for constructing LazyString, e.g. lazy"$x $op $y overflowed for type $(typeof(x))"?

@tkf
Copy link
Member Author

tkf commented Apr 12, 2022

I'm pretty sure I tried it but the macro didn't work at this phase during bootstrap.

@aviatesk aviatesk merged commit 7f121e6 into JuliaLang:master Apr 12, 2022
@tkf tkf deleted the effect-free-gcd branch April 12, 2022 13:42
@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Jul 25, 2022
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Aug 7, 2022
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.

4 participants