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

Work around #29952. #29953

Closed
wants to merge 2 commits into from
Closed

Work around #29952. #29953

wants to merge 2 commits into from

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Nov 7, 2018

Proposed hotfix for JuliaGPU/CUDAnative.jl#278, easy to backport. print_to_string is a very common function; even though in the case of GPU code it typically doesn't get emitted or optimized away, after #28876 it requires recursion which is GPU incompatible. Unless of course there's a easy fix for #29952.

@maleadt maleadt added bugfix This change fixes an existing bug backport pending 1.0 labels Nov 7, 2018
@KristofferC
Copy link
Member

Could we put in a comment here.

@KristofferC KristofferC added the priority This should be addressed urgently label Nov 7, 2018
@KristofferC
Copy link
Member

KristofferC commented Nov 7, 2018

This adds an extra allocation to string

julia> @btime string(3, "f") # PR
  260.592 ns (6 allocations: 272 bytes)
"3f"

julia> @btime string(3, "f") # 1.0.1
  229.043 ns (5 allocations: 256 bytes)
"3f"

But I guess it is worth it :/

@JeffBezanson
Copy link
Member

Ok, fine, but this seems pretty brittle. It's also not great that we have these recursion cycles everywhere because of string concatenation to throw errors in low-level routines. We will have to reckon with that somehow.

@maleadt
Copy link
Member Author

maleadt commented Nov 8, 2018

Ok, fine, but this seems pretty brittle.

Yeah, this is just to have the upcoming 1.0.2 release GPU compatible. We'll have to find a better fix.

@maleadt
Copy link
Member Author

maleadt commented Nov 8, 2018

Although fixing recursion might not be very hard. Observing it comes from print_to_string constructing a IOBuffer whose inner constructor can throw a ArgumentException with string interpolation (itself calling string->print_to_string again):

diff --git a/base/iobuffer.jl b/base/iobuffer.jl
index 7633581508..253f42e1d3 100644
--- a/base/iobuffer.jl
+++ b/base/iobuffer.jl
@@ -89,7 +89,7 @@ function IOBuffer(
         maxsize::Integer=typemax(Int),
         sizehint::Union{Integer,Nothing}=nothing)
     if maxsize < 0
-        throw(ArgumentError("negative maxsize: $(maxsize)"))
+        throw(ArgumentError("negative maxsize"))
     end
     if sizehint !== nothing
         sizehint!(data, sizehint)

Looks more "reasonable", at least. Also goes back to 5 allocations for calling string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug priority This should be addressed urgently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants