JuliaGraphics / Luxor.jl

Simple drawings using vector graphics; Cairo "for tourists!"
http://juliagraphics.github.io/Luxor.jl/
Other
576 stars 72 forks source link

Thread safety #238

Closed oheil closed 1 year ago

oheil commented 1 year ago

Issue https://github.com/JuliaGraphics/Luxor.jl/issues/117

Luxor is now thread safe. This was quite complex, but it doesn't change anything for existing users and all old and new tests are fine.

I tried but failed to use https://github.com/tkf/ContextVariablesX.jl . I think it's because Luxor not only needs a context var x but a set of arrays for multi drawing. So I separated the globals using the thread ID, which is not perfect. A can't find a better way. If someone could show how it can be done better this would be great.

If users don't want anything of this fancy multi drawing or threads, nothing changes for them. Luxor stays the simplified version of Cairo.

Of course the changes seem to be massive. Any questions and discussion is welcome.

codecov[bot] commented 1 year ago

Codecov Report

Merging #238 (73c68cd) into master (5f9e24c) will increase coverage by 0.08%. The diff coverage is 80.90%.

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
+ Coverage   73.60%   73.69%   +0.08%     
==========================================
  Files          32       32              
  Lines        6203     6261      +58     
==========================================
+ Hits         4566     4614      +48     
- Misses       1637     1647      +10     
Impacted Files Coverage Δ
src/latex.jl 78.65% <42.85%> (ø)
src/drawings.jl 67.34% <82.02%> (+1.21%) :arrow_up:
src/basics.jl 92.27% <92.85%> (-0.03%) :arrow_down:
src/animate.jl 79.39% <0.00%> (+1.81%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5f9e24c...73c68cd. Read the comment docs.

oheil commented 1 year ago

Would be good if someone else who knows about thread stuff could review this

Let's ping Takafumi Arakaki ( @tkf ) as I refer to his original issue and wasn't able to use his package ContextVariablesX.jl . Maybe he don't mind to have a look at my proposal.

Meanwhile I work on the other good points.

oheil commented 1 year ago

Hm, seems not related to my changes... any idea? just start again?

oheil commented 1 year ago
(@v1.7) pkg> activate test
  Activating project at `C:\Users\Oli\test`

julia> using Luxor, Test, LaTeXStrings, MathTeXEngine

julia>     d = Drawing(200,200,:svg)
 Luxor drawing: (type = :svg, width = 200.0, height = 200.0, location = in memory)

julia>     origin()

julia>     fontsize(18)

julia>     t = L"\sum^N_{n=1} \frac{x}{n}"
L"$\sum^N_{n=1} \frac{x}{n}$"

julia> halign=:left
:left

julia> valign=:baseline
:baseline

julia> text(t, halign=halign, valign=valign)
ERROR: type TeXChar has no field char
Stacktrace:
 [1] getproperty
   @ .\Base.jl:42 [inlined]
 [2] writelatexchar(text::Tuple{TeXChar, GeometryBasics.Point2{Float64}, Float64}, font_size::Float64)
   @ Luxor C:\Users\Oli\.julia\packages\Luxor\dynCE\src\latex.jl:196
 [3] macro expansion
   @ C:\Users\Oli\.julia\packages\Luxor\dynCE\src\latex.jl:160 [inlined]
 [4] macro expansion
   @ C:\Users\Oli\.julia\packages\Luxor\dynCE\src\basics.jl:526 [inlined]
 [5] text(lstr::LaTeXString, pt::Point; valign::Symbol, halign::Symbol, angle::Int64, rotationfixed::Bool, paths::Bool, kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Luxor C:\Users\Oli\.julia\packages\Luxor\dynCE\src\latex.jl:136
 [6] #text#226
   @ C:\Users\Oli\.julia\packages\Luxor\dynCE\src\text.jl:83 [inlined]
 [7] top-level scope
   @ REPL[9]:1

julia> versioninfo()
Julia Version 1.7.2
Commit bf53498635 (2022-02-06 15:21 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: AMD Ryzen 9 3900X 12-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.1 (ORCJIT, znver2)

(test) pkg> st
      Status `C:\Users\Oli\test\Project.toml`
  [5ae59095] Colors v0.12.8
  [b964fa9f] LaTeXStrings v1.3.0
  [ae8d54c2] Luxor v3.4.0
  [0a4f8689] MathTeXEngine v0.5.0
  [cc649173] MiniFB v0.1.1
cormullion commented 1 year ago

I think MathTexEngine.jl was updated to v0.5 15 hours ago, looks like the LaTex code broke somehow... Perhaps we should stick with v0.4.0 for now.

oheil commented 1 year ago

I don't know if these are proper changes to the latex code...

cormullion commented 1 year ago

let's give it a go...

cormullion commented 1 year ago

@oheil Thanks for the great contributions!

guo-yong-zhi commented 1 year ago

I found that it doesn't work for settext. If I delete the settext line, the code runs fine.

using Luxor

function make_drawing()
    Drawing(200, 200, :image)
    setcolor(1, 0, 0)
    poly([Point(1,10), Point(100,110), Point(50,50)], close=true, action=:fill) 
    settext("abcd", Point(50,50), halign="center", valign="center")
    m = image_as_matrix()    
    finish()
    return m
end
Threads.@threads for i = 1:1000
    make_drawing()
end

(process:22772): Pango-WARNING : 11:31:06.236: couldn't load font "serif Not-Rotated 12", falling back to "Sans Not-Rotated 12", expect ugly output. (process:22772): Pango-WARNING : 11:31:06.236: couldn't load font "Sans Not-Rotated 12", falling back to "Sans Not-Rotated 12", expect ugly output. ...

versioninfo()

Julia Version 1.7.2 Commit bf53498635 (2022-02-06 15:21 UTC) Platform Info: OS: Windows (x86_64-w64-mingw32) CPU: Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz WORD_SIZE: 64 LIBM: libopenlibm LLVM: libLLVM-12.0.1 (ORCJIT, skylake) Environment: JULIA_NUM_THREADS = auto

]st

[ae8d54c2] Luxor v3.5.0 [36c8627f] Pango_jll v1.42.4+10 ⚲

cormullion commented 1 year ago

It works OK for me:

using Luxor

function make_drawing(i)
    Drawing(200, 200, "/tmp/$(i).png")
    origin()
    background("black")
    randomhue()
    ngon(O, 90, 3 + i % 4, 0, :fill)
    sethue("white")
    setfont("Aachen", 120)
    settext("$(Threads.threadid())", O, halign="center", valign="center")
    finish()
end

Threads.@threads for i = 1:100
    make_drawing(i)
end
Screenshot 2022-07-30 at 08 35 42

Although I wouldn't have been too surprised (the word "pango" has that effect), it appears to be working on my machine.

oheil commented 1 year ago

I will investigate...

oheil commented 1 year ago

Confirmed:

(process:15388): Pango-WARNING **: 12:21:20.224: couldn't load font "Aachen Not-Rotated 90", falling back to "Sans Not-Rotated 90", expect ugly output.

only occurs if in threaded version. Why is this here and not in issues? Can this be splited into issues? If not I make an issue...

guo-yong-zhi commented 1 year ago

My bad. Sure, it should be an issue, please ~split it~ make one. @oheil