lock by default - add safe_lock option for control

[lmiq]

This is another possible fix for: #317 and #232

Here we remove the automatic identification of threading, and always use a lock by default. An option safe_lock is added, which is by default true and must be set to false to disable the lock.

Relative to the current stable version I don't see any measurable performance difference:

Current stable:

julia> function prog_perf(n)
           prog = Progress(n)
           x = 0.0
           for i in 1:n
               x += rand()
               next!(prog)
           end
           return x
       end
prog_perf (generic function with 1 method)

julia> @time prog_perf(10^7)
Progress: 100%|█████████████████████████████████████████████████████| Time: 0:00:05
  5.938973 seconds (122.22 M allocations: 1.889 GiB, 2.62% gc time, 28.52% compilation time)
4.999510125125184e6

julia> @time prog_perf(10^7)
Progress: 100%|█████████████████████████████████████████████████████| Time: 0:00:03
  3.953561 seconds (119.76 M allocations: 1.785 GiB, 2.68% gc time)
4.999360964692622e6

julia> @time prog_perf(10^7)
Progress: 100%|█████████████████████████████████████████████████████| Time: 0:00:04
  4.044011 seconds (119.74 M allocations: 1.784 GiB, 2.60% gc time)
5.0003511864208765e6

With this PR:

julia> function prog_perf(n; safe_lock=true)
           prog = Progress(n; safe_lock)
           x = 0.0
           for i in 1:n
               x += rand()
               next!(prog)
           end
           return x
       end
prog_perf (generic function with 1 method)

julia> @time prog_perf(10^7)
Progress: 100%|█████████████████████████████████████████████████████| Time: 0:00:04
  4.068567 seconds (119.76 M allocations: 1.785 GiB, 4.76% gc time)
5.002101485254395e6

julia> @time prog_perf(10^7)
Progress: 100%|█████████████████████████████████████████████████████| Time: 0:00:03
  3.992197 seconds (119.74 M allocations: 1.784 GiB, 3.91% gc time)
4.999887783558271e6

julia> @time prog_perf(10^7; safe_lock=false)
Progress: 100%|█████████████████████████████████████████████████████| Time: 0:00:03
  3.704703 seconds (119.74 M allocations: 1.784 GiB, 4.53% gc time)
4.998432809492471e6

julia> @time prog_perf(10^7; safe_lock=false)
Progress: 100%|█████████████████████████████████████████████████████| Time: 0:00:03
  3.749583 seconds (119.73 M allocations: 1.784 GiB, 4.10% gc time)
5.000024621577769e6

There is an apparent small but consistent advantage when safe_lock=false, which is expected.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.73%. Comparing base (807496a) to head (6cf497c).
Report is 14 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
+ Coverage   93.48%   96.73%   +3.24%     
==========================================
  Files           1        1              
  Lines         399      551     +152     
==========================================
+ Hits          373      533     +160     
+ Misses         26       18       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MarcMush]

looks good

[lmiq]

Shoudn´t we release one of these fixes? None is breaking, and while the ˋsafe_lockˋ option is not documented, it can be reverted. If a better solution if proposed later, it can be implemented anyway.

[lmiq]

In #232 (comment) @IanButterworth mentions that the use of the progress meter slows down 9x the tight loop. I actually see a much greater slowdown with the released version, but my tests with this version, for which the lock can be removed, show that the slowdown is not really related to the lock.

[MarcMush]

the performance hit is slightly more significant on my machine (windows maybe)

julia> @time prog_perf(10^7);
Progress: 100%|█████████████████████████████████████| Time: 0:00:07
  7.290191 seconds (130.46 M allocations: 2.093 GiB, 0.73% gc time)

julia> @time prog_perf(10^7; safe_lock=false);
Progress: 100%|█████████████████████████████████████| Time: 0:00:06
  6.235205 seconds (120.35 M allocations: 1.794 GiB, 0.40% gc time)

maybe the default could be safe_lock=nthreads()>1

[lmiq]

The main issue I see here is that the comparison with not using the progress meter is so great, that it makes me wander if this discussion about performance is really relevant. This is what I get here with the functions used in the test suite for this:

julia> @time noprog_perf(10^7)
  0.014616 seconds
5.001148339582725e6

julia> @time prog_perf(10^7)
Progress: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| Time: 0:00:05
  5.515208 seconds (119.79 M allocations: 1.785 GiB, 0.55% gc time)
5.000329702123166e6

julia>

This is with the current released version. Does it make sense to discuss the performance differences of using or not a lock in this context? Seems to me that using it for a tight loop simply does not make sense with the current status of things.

Edit: this is just something to think about: a very simple progress bar, with a lock, and the time it takes:

julia> function simple_prog_perf(n)
           lk = ReentrantLock()
           print("\n Starting: ")
           x = 0.0
           for i in 1:n
               x += rand()
               mod(i,div(n,80)) == 0 && @lock lk print("-")
           end
           println(": Finish")
           return x
       end
simple_prog_perf (generic function with 1 method)

julia> @time simple_prog_perf(10^7)

 Starting: --------------------------------------------------------------------------------: Finish
  0.036811 seconds (91 allocations: 2.312 KiB)
4.999961453282997e6

If we want to provide an efficient progress bar for this kind of loop, perhaps some option along these lines is necessary.

[MarcMush]

I can merge this once the tests are fixed, if noone objects to this

[lmiq]

Tests fixed.