KristofferC
commented
7 months ago Can you profile it? Potentially fixed by https://github.com/JuliaLang/Pkg.jl/pull/3823.
Open t-bltg opened 7 months ago
KristofferC
commented
7 months ago Can you profile it? Potentially fixed by https://github.com/JuliaLang/Pkg.jl/pull/3823.
t-bltg
commented
7 months ago Thanks, running from the REPL doesn't regress locally, so you are probably right about https://github.com/JuliaLang/Pkg.jl/pull/3823 !
I'll keep an eye on the next alpha or beta version of 1.11.0 then ...
using UnicodePlots
let p = heatmap(collect(1:30) * collect(1:30)')
string(p; color = true) # 1st pass - ttfp
GC.enable(false)
stats = @timed string(p; color = true) # repeated !
GC.enable(true)
@show stats.bytes stats.time
end
# stats.bytes = 372920 stats.time = 0.000797776 (1.11.0-alpha1)
# stats.bytes = 355704 stats.time = 0.000682859 (1.10.2)AFAIK this is still broken on 1.11.0-alpha2, running pkg> test UnicodePlots locally.
IanButterworth
commented
6 months ago Yeah a profile would help here. You'd need to do it in the test suite itself
t-bltg
commented
6 months ago Added in the test suite:
p = heatmap(collect(1:30) * collect(1:30)')
string(p; color = true) # 1st pass
GC.enable(false)
Profile.init(; n=10^9, delay=1e-7)
Profile.clear()
@profile string(p; color = true)
Profile.print()
GC.enable(true)Here is the Profile output on 1.10.2:
And on 1.11.0-alpha2:
IanButterworth
commented
6 months ago Thanks. Can you try with and without coverage on? (It looks like it's currently on in the above cases?)
KristofferC
commented
6 months ago If I put
using UnicodePlots, Test
using BenchmarkTools
p = heatmap(collect(1:30) * collect(1:30)')
@btime string(p; color = true) in runtests.jl I cannot observe a difference if I run with Pkg.test or including it. Did you manage to repro it at all locally?
(UnicodePlots) pkg> test
Testing UnicodePlots
Status `/tmp/jl_GnheYo/Project.toml`
[4c88cf16] Aqua v0.8.4
[6e4b80f9] BenchmarkTools v1.5.0
...
[3f19e933] p7zip_jll v17.4.0+2
Info Packages marked with ⌃ and ⌅ have new versions available. Those with ⌃ may be upgradable, but those with ⌅ are restricted by compatibility constraints from upgrading.
Testing Running tests...
218.893 μs (11194 allocations: 364.15 KiB)
Testing UnicodePlots tests passed
julia> include("test/runtests.jl")
208.180 μs (11194 allocations: 364.15 KiB)Are you on 1.11.0-alpha2 ?
(UnicodePlots) pkg> test fails locally
This also fails locally:
using TestEnv
TestEnv.activate()
include("test/runtests.jl")Given coverage is on on CI, I tried moving the load of UnicodePlots last in case it was due to https://github.com/JuliaLang/julia/pull/53457
but it still fails https://github.com/JuliaPlots/UnicodePlots.jl/pull/375
So doesn't seem to be due to that coverage issue.
And you're reporting it locally without coverage turned on, I believe
t-bltg
commented
6 months ago @IanButterworth, thanks for trying this !
KristofferC
commented
6 months ago
(UnicodePlots) pkg> testfails locally
Ok, but I am asking for something smaller that shows the test case in https://github.com/JuliaLang/julia/issues/53688#issuecomment-2009791553 being slower.
t-bltg
commented
6 months ago And you're reporting it locally without coverage turned on, I believe
I've tested:
using TestEnv
TestEnv.activate()
include("test/runtests.jl")with --code-coverage and with --code-coverage=none, and this doesn't make a difference.
t-bltg
commented
6 months ago Oddly, when I move the definition of the function sombrero outside the @testset block, the regression disappears.
So this appears to be a Test module issue / regression, anyway I have a workaround now.
KristofferC
commented
6 months ago That's a useful observation. It shouldn't be too hard to bisect it from that.
IanButterworth
commented
6 months ago I tried testing this locally on nightly and didn't see the test failure. That was with e07c0f1ddbfc89ad1ac4dda7246d8ed5d0d57c19 and UnicodePlots v3.6.4 (before the fix) and https://github.com/FedeClaudi/Term.jl/pull/245 which was required due to https://github.com/JuliaLang/julia/pull/53704 (maybe that type stability fix helped?)
t-bltg
commented
6 months ago Thanks @IanButterworth , I'll try to bisect on 1.11 next week.
I have a little test case used to monitor allocations regression and runtime performance in
UnicodePlotsCI (with some tolerance): https://github.com/JuliaPlots/UnicodePlots.jl/blob/main/test/tst_io.jl#L67-L69.It's been working flawlessly since julia
1.6and up-to julia1.10.2, but since1.11.0-alpha1there seems to be a massive regression in time performance (8ms vs 0.3ms before) and slightly more allocations (500kB vs 356kB before), resulting in CI failure such as the following: https://github.com/JuliaPlots/UnicodePlots.jl/actions/runs/8173876886/job/22347317855#step:6:375