-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Make gcd/lcm effect-free by using LazyString #44935
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
Conversation
|
Does this increase the amount of codegen for errors? Should we start reporting the value for all our 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. |
| throw(OverflowError(Base.invokelatest(string, x, " ", op, " ", y, " overflowed for type ", typeof(x))))) | ||
| throw(OverflowError(LazyString(x, " ", op, " ", y, " overflowed for type ", typeof(x))))) |
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 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?
Line 28 in 0deb326
| LazyString(args...) = new(args) |
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.
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.
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.
Do we want to use lazy"..." syntax for constructing LazyString, e.g. lazy"$x $op $y overflowed for type $(typeof(x))"?
|
I'm pretty sure I tried it but the macro didn't work at this phase during bootstrap. |
Before:
After:
When playing with auto-parallelization (#43910, https://github.com/JuliaFolds/ParallelMagics.jl), I wanted to use
gcdin 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.