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

Improve finish! behavior for Progress #198

Merged
merged 1 commit into from Apr 23, 2021
Merged

Improve finish! behavior for Progress #198

merged 1 commit into from Apr 23, 2021

Conversation

halleysfifthinc
Copy link
Contributor

When finish! is called on a Progress type meter with a slow update limit and a significant number of steps left, it will max a CPU core while slowly updating the progress meter until it is complete.

MWE:

julia> p = Progress(100000000; dt=.1);

julia> next!(p); @time finish!(p)
Progress: 100%|██████████████████████| Time: 0:00:16
 14.919932 seconds (6.75 k allocations: 357.250 KiB)

This took me by surprise, and doesn't match the immediate completion behavior specified in the code for the finish! methods for ProgressThresh and ProgressUnknown.

PR:

julia> p = Progress(100000000; dt=.1);

julia> next!(p); @time finish!(p)
Progress: 100%|██████████████████████| Time: 0:00:01
  0.000068 seconds (43 allocations: 3.422 KiB)

@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@80acddf). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #198   +/-   ##
=========================================
  Coverage          ?   97.27%           
=========================================
  Files             ?        1           
  Lines             ?      587           
  Branches          ?        0           
=========================================
  Hits              ?      571           
  Misses            ?       16           
  Partials          ?        0           
Impacted Files Coverage Δ
src/ProgressMeter.jl 97.27% <100.00%> (ø)

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 80acddf...0823129. Read the comment docs.

@IanButterworth
Copy link
Collaborator

IanButterworth commented Apr 22, 2021

Indeed! This will have been especially hurt by the increase in lock-unlock speed in recent julia versions.

It seems like it should always have been this way. Thanks

@IanButterworth
Copy link
Collaborator

There was a Windows CI hanging bug that #195 just fixed, so I'll re-run the merge commit by closing & opening

@IanButterworth
Copy link
Collaborator

It seems like that didn't actually test the merge commit given CI passes on master. I'll merge

@IanButterworth IanButterworth merged commit abcfc04 into timholy:master Apr 23, 2021
@halleysfifthinc halleysfifthinc deleted the patch-1 branch April 23, 2021 15:10
@IanButterworth IanButterworth mentioned this pull request Apr 25, 2021
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.

None yet

2 participants