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

added a buffer for unfinished lines during progress bar printing. the lines will be completed by the next print, or way at the end when all progress bars are done. #1951

Merged
merged 8 commits into from
May 24, 2024

Conversation

jurgenvinju
Copy link
Member

@jurgenvinju jurgenvinju commented May 23, 2024

Fixes #1948 and #1946 by considering all different situations that can arise when output is printed during progress bar visualizations:

  • the new output does not end with a newline
  • previously output did not end with a newline
  • both combined, and
  • neither

The new implementation satisfies the invariant that the cursor position will always be on column 0 when a new version of a progress bar is printed. This is necesary for the progress bar to fit one exactly one line and fill it; and also to prevent flickering (a bar that moves would flicker).

Furthermore this implementation is thread-safe, because it maintains unfinished sentences for every different thread, and completes them incrementally as new output is printed from each thread. In this manner another invariant is satisfied: never is output printed one the same line from different threads. The lines of different threads may interleave; but not the columns.

The printing is now incremental per line such that output still appears as the program is running, but it is synchronized on newline characters, in order to be able to satisfy the "column=0" invariant at all times.

… lines will be completed by the next print, or way at the end when all progress bars are done. Also removed some unused imports
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 52 lines in your changes are missing coverage. Please review.

Project coverage is 49%. Comparing base (fe4323a) to head (6a30669).
Report is 2 commits behind head on main.

Current head 6a30669 differs from pull request most recent head f3a56ad

Please upload reports for the commit f3a56ad to get more accurate results.

Files Patch % Lines
...org/rascalmpl/repl/TerminalProgressBarMonitor.java 0% 52 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              main   #1951   +/-   ##
=======================================
- Coverage       49%     49%   -1%     
- Complexity    6247    6248    +1     
=======================================
  Files          664     664           
  Lines        59464   59512   +48     
  Branches      8621    8629    +8     
=======================================
- Hits         29326   29324    -2     
- Misses       27943   27990   +47     
- Partials      2195    2198    +3     

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

@jurgenvinju jurgenvinju marked this pull request as ready for review May 23, 2024 11:02
@jurgenvinju jurgenvinju requested a review from DavyLandman May 23, 2024 11:08
@jurgenvinju
Copy link
Member Author

This definitely needs testing on Windows; due to additional \r characters. However dependence on Windows/Unix characteristics is becoming less due to this fix, since we only print if we have a newline character and \r always becomes before \n in Windows.

@jurgenvinju jurgenvinju changed the title added a buffer for unfinished lines during progress bar printing. the lines will be completed by the next print, or way at the end when all progress bars are done. Also removed some unused imports added a buffer for unfinished lines during progress bar printing. the lines will be completed by the next print, or way at the end when all progress bars are done. May 23, 2024
@jurgenvinju
Copy link
Member Author

jurgenvinju commented May 23, 2024

Efficiency could be improved by making the buffer circular; however I noticed no performance hit with in the horseRaceTest demo, so this could be goldplating.

@DavyLandman
Copy link
Member

I've tested it in 2 windows terminals:

  • the standard windows one:

rascal>:test










☑ Testing $shell$                                                                                                                                                                                                                             🕒 0:00:00.017
[WARNING] Monitor of Horse 0 (handicap is 0) is over max (3000000) by 4
☑ Horse 0 (handicap is 0)                                                                                                                                                                                                                     🕘 0:00:03.021
☐ Horse 1 (handicap is 5)                                                                                                                                                                                                                     🕓 0:00:03.021
☐ Horse 2 (handicap is 4)                                                                                                                                                                                                                     🕐 0:00:03.020
☐ Horse 3 (handicap is 3)                                                                                                                                                                                                                     🕓 0:00:03.020
☐ Horse 4 (handicap is 2)                                                                                                                                                                                                                     🕐 0:00:03.021




☑ Job                                                                                                                                                                                                                                         🕔 0:00:00.002
☑ Job                                                                                                                                                                                                                                         🕕 0:00:00.011


☑ Testing util::Monitor                                                                                                                                                                                                                       🕔 0:00:03.046
bool: false
rascal>
  • The new microsoft terminal (non default):
rascal>:test
a
b
c
d
☑ Job                                                                                                   🕔 0:00:00.007
a
bcdefghij
☑ Job                                                                                                   🕕 0:00:00.029
[WARNING] Monitor of Horse 3 (handicap is 0) is over max (3000000) by 6
☐ Horse 0 (handicap is 6)                                                                               🕑 0:00:03.278
☐ Horse 1 (handicap is 2)                                                                               🕘 0:00:03.279
☐ Horse 2 (handicap is 1)                                                                               🕑 0:00:03.279
☑ Horse 3 (handicap is 0)                                                                               🕔 0:00:03.278
☐ Horse 4 (handicap is 5)                                                                               🕕 0:00:03.278
Test report for util::Monitor
        all 3/3 tests succeeded
☑ Testing util::Monitor                                                                                 🕔 0:00:03.340
bool: true
rascal>

So it's partially works, but we're not there yet?

Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I think the logic in the UnfinishedLine can be improved.

src/org/rascalmpl/repl/TerminalProgressBarMonitor.java Outdated Show resolved Hide resolved
src/org/rascalmpl/repl/TerminalProgressBarMonitor.java Outdated Show resolved Hide resolved
// first ensure capacity of the array
if (curEnd + len >= curCapacity) {
var oldCapacity = curCapacity;
curCapacity *= 2; // this should not happen to often. we're talking a few lines of text.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should grow by 50%?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the chance of having to grow again is large if we only grow by 50%. This way we quickly grow and we don't grow again on short notice.

src/org/rascalmpl/repl/TerminalProgressBarMonitor.java Outdated Show resolved Hide resolved
src/org/rascalmpl/repl/TerminalProgressBarMonitor.java Outdated Show resolved Hide resolved
// otherwise we wait until the next input comes to be able to complete a line.
}

public void writeLeftOvers(OutputStream out) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to flush?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the same as a flush because it also adds newlines. So I have renamed it.

src/org/rascalmpl/repl/TerminalProgressBarMonitor.java Outdated Show resolved Hide resolved
@jurgenvinju
Copy link
Member Author

So it's partially works, but we're not there yet?

I think we might be looking at a different problem there that is not related to the current solution. Perhaps it is missing \r's or adding extra \r's and therefore ruining the visual output.

…pying of byte arrays that will be printed immediately anyway. The code now only buffers unfinished lines.
@jurgenvinju jurgenvinju merged commit 35bbe87 into main May 24, 2024
1 check passed
@jurgenvinju jurgenvinju deleted the unfinished-lines-during-progress-bars branch May 24, 2024 14:21
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.

Unfinished output during progress bar printing leads to missing output.
2 participants