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

nospecialize in displaying SOneTo #1198

Merged
merged 1 commit into from
Oct 31, 2023
Merged

nospecialize in displaying SOneTo #1198

merged 1 commit into from
Oct 31, 2023

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Sep 20, 2023

No description provided.

@mateuszbaran
Copy link
Collaborator

For some weird reason this makes printing slower?
Current StaticArrays:

julia> @time repr(SOneTo(3))
  0.002636 seconds (20 allocations: 1.133 KiB, 99.22% compilation time)
"SOneTo(3)"

julia> @time repr(SOneTo(4))
  0.004234 seconds (20 allocations: 1.133 KiB, 99.47% compilation time)
"SOneTo(4)"

julia> function Base.show(io::IO, @nospecialize(x::SOneTo))
           print(io, "SOneTo(", length(x)::Int, ")")
       end

julia> @time repr(SOneTo(3))
  0.007384 seconds (1.04 k allocations: 71.046 KiB, 99.67% compilation time)
"SOneTo(3)"

julia> @time repr(SOneTo(4))
  0.000025 seconds (7 allocations: 344 bytes)
"SOneTo(4)"

@jishnub
Copy link
Member Author

jishnub commented Sep 22, 2023

I'd say that this PR improves things, as the compilation happens only once, and subsequent calls are fast. Perhaps the first run is slowed down by the nospecialize, but performance is usually not a big concern in displaying values anyway.

In case this matters, the first call may be precompiled using PrecompileTools, in which case no subsequent call will require compilation.

@jishnub
Copy link
Member Author

jishnub commented Oct 31, 2023

Gentle bump

@mateuszbaran
Copy link
Collaborator

#1187 is now merged so I think we could add printing of SOneTo to compile_workload and it would be completely fine.

@jishnub
Copy link
Member Author

jishnub commented Oct 31, 2023

I'm unsure what's the best way to address this, as show(IOBuffer(), SOneTo(1)) doesn't precompile it for displaying to a terminal. What works is show(SOneTo(1)), but this writes to the terminal during precompilation. I've tried to redirect the stdin to devnull, but this leads to an error on my system.

@mateuszbaran
Copy link
Collaborator

I think you should redirect stdout, not stdin. Like this:

redirect_stdout(devnull) do
    show(SOneTo(1))
end

in the workload.

@jishnub
Copy link
Member Author

jishnub commented Oct 31, 2023

Thanks for catching that! Oddly, adding this doesn't improve the compilation time. I wonder if this is because the display to Base.TTY is not compiled when the output is redirected to devnull, which is a Base.DevNull

@mateuszbaran
Copy link
Collaborator

I see. Well, let's merge this then without precompilation.

@hyrodium hyrodium merged commit 67d9c36 into master Oct 31, 2023
22 of 27 checks passed
@hyrodium hyrodium deleted the jishnub-patch-1 branch October 31, 2023 13:55
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