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.

Sign up for GitHub

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

Jump to bottom

Only flush output frequently for ProgressFormatter #2541

Merged
merged 8 commits into from
Apr 30, 2022
Merged

Only flush output frequently for ProgressFormatter #2541

merged 8 commits into from
Apr 30, 2022

Conversation

scottadav
Copy link
Contributor

🤔 What's changed?

  • Add a flag to NIceAppendable to configure if it should flush on every method call
  • Add unit tests to support the change to NIceAppendable
  • Remove the "flush on every write" behavior for all the formatters except ProgressFormatter
  • Leave the "flush on every write" behavior in-place for ProgressFormatter
  • Modify UnusedStepsSummaryPrinter to close its appender upon receiving the TestRunFinished event
    • This was modified to make the unit tests continue to pass. However, closing the appender upon TestRunFinished is consistent with all the other ConcurrentEventListener's, and it will prevent leaking system resources.

⚡️ What's your motivation?

This fixes #2481 and #2536

I am only concerned with #2536, but this fixes both issues at once.

🏷️ What kind of change is this?

  • 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

Just make sure it meets project standards and that the change looks acceptable to the reviewers. I'm don't know the project's branching standards. Feel free to modify anything as needed, or you can send it back to me with instructions. Whatever works. Thanks!

📋 Checklist:

This text was originally generated from a template, then edited by hand. You can modify the template here.

This was referenced Apr 29, 2022
Closed
Closed
mpkorstanje
mpkorstanje reviewed Apr 29, 2022
View reviewed changes
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Looks good. Some nitpicks. Don't forget to add an entry to the changelog.

core/src/main/java/io/cucumber/core/plugin/NiceAppendable.java Show resolved Hide resolved
core/src/main/java/io/cucumber/core/plugin/UnusedStepsSummaryPrinter.java Show resolved Hide resolved
core/src/main/java/io/cucumber/core/plugin/ProgressFormatter.java Show resolved Hide resolved
@scottadav
Copy link
Contributor Author

Changelog entry added by commit 6d01075

@mpkorstanje mpkorstanje changed the title Fix #2481 - Only flush output frequently for ProgressFormatter Only flush output frequently for ProgressFormatter Apr 30, 2022
@codecov
Copy link

codecov bot commented Apr 30, 2022 •
edited
Loading

Codecov Report

Merging #2541 (d2722a8) into main (549fe86) will increase coverage by 0.04%.
The diff coverage is 91.30%.

@@             Coverage Diff              @@
##               main    #2541      +/-   ##
============================================
+ Coverage     84.45%   84.50%   +0.04%     
- Complexity     2651     2655       +4     
============================================
  Files           310      310              
  Lines          9324     9337      +13     
  Branches        896      898       +2     
============================================
+ Hits           7875     7890      +15     
+ Misses         1116     1115       -1     
+ Partials        333      332       -1
Impacted Files Coverage Δ
...ava/io/cucumber/core/plugin/ProgressFormatter.java 90.00% <84.61%> (+12.85%) ⬆️
...n/java/io/cucumber/core/plugin/NiceAppendable.java 59.09% <100.00%> (+5.24%) ⬆️
.../java/io/cucumber/core/plugin/PrettyFormatter.java 85.31% <100.00%> (+0.20%) ⬆️
...ucumber/core/plugin/UnusedStepsSummaryPrinter.java 86.66% <100.00%> (+0.45%) ⬆️
...main/java/io/cucumber/core/plugin/AnsiEscapes.java 88.00% <0.00%> (-8.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 549fe86...d2722a8. Read the comment docs.

@mpkorstanje mpkorstanje merged commit 4605c55 into cucumber:main Apr 30, 2022
@aslakhellesoy
Copy link
Contributor

Hi @scottadav,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@mpkorstanje
Copy link
Contributor

Cheers!

@rustemsam rustemsam mentioned this pull request May 31, 2022
Closed
@aslakhellesoy aslakhellesoy mentioned this pull request Jun 6, 2022
Closed
mpkorstanje added a commit that referenced this pull request Jun 21, 2022
@mpkorstanje
[Core] Flush pretty output manually
e8a7bb5 
For the sake of efficiency and performance we write to buffered writers.
However, the pretty plugin is expected to print scenarios as they're started
and steps as they're finished.

This was initially resolved by flushing the `NiceAppendable` on every write.
This turned out to have performance impact (#2481) and interact poorly with
tools that expected (#2536) us to be better behaved.

By having `NiceAppendable` flush only when the underlying buffer was full these
problems were resolved (#2541) . However, because the underlying buffer is 8KiB
the pretty formatters output would effectively sit in a buffer until the writer
was closed.

So now we flush the pretty and progress output manually after each event. This
strikes a balance somewhere between the flush-everything-all-the-time and
flush-when-full approaches.

Fixes: #2563
@mpkorstanje mpkorstanje mentioned this pull request Jun 21, 2022
Merged
5 tasks
mpkorstanje added a commit that referenced this pull request Jun 21, 2022
@mpkorstanje
[Core] Flush pretty output manually
78a6fd1 
For the sake of efficiency and performance we write to buffered writers.
However, the pretty plugin is expected to print scenarios as they're started
and steps as they're finished.

This was initially resolved by flushing the `NiceAppendable` on every write.
This turned out to have performance impact (#2481) and interact poorly with
tools that expected (#2536) us to be better behaved.

By having `NiceAppendable` flush only when the underlying buffer was full these
problems were resolved (#2541) . However, because the underlying buffer is 8KiB
the pretty formatters output would effectively sit in a buffer until the writer
was closed.

So now we flush the pretty and progress output manually after each event. This
strikes a balance somewhere between the flush-everything-all-the-time and
flush-when-full approaches.

Fixes: #2563
mpkorstanje added a commit that referenced this pull request Jun 21, 2022
@mpkorstanje
[Core] Flush pretty output manually
27e7a0f 
For the sake of efficiency and performance we write to buffered writers.
However, the pretty plugin is expected to print scenarios as they're started
and steps as they're finished.

This was initially resolved by flushing the `NiceAppendable` on every write.
This turned out to have performance impact (#2481) and interact poorly with
tools that expected (#2536) us to be better behaved.

By having `NiceAppendable` flush only when the underlying buffer was full these
problems were resolved (#2541) . However, because the underlying buffer is 8KiB
the pretty formatters output would effectively sit in a buffer until the writer
was closed.

So now we flush the pretty and progress output manually after each event. This
strikes a balance somewhere between the flush-everything-all-the-time and
flush-when-full approaches.

Fixes: #2563
mpkorstanje added a commit that referenced this pull request Jun 21, 2022
@mpkorstanje
[Core] Flush pretty output manually
f9e6e75 
For the sake of efficiency and performance we write to buffered writers.
However, the pretty plugin is expected to print scenarios as they're started
and steps as they're finished.

This was initially resolved by flushing the `NiceAppendable` on every write.
This turned out to have performance impact (#2481) and interact poorly with
tools that expected (#2536) us to be better behaved.

By having `NiceAppendable` flush only when the underlying buffer was full these
problems were resolved (#2541) . However, because the underlying buffer is 8KiB
the pretty formatters output would effectively sit in a buffer until the writer
was closed.

So now we flush the pretty and progress output manually after each event. This
strikes a balance somewhere between the flush-everything-all-the-time and
flush-when-full approaches.

Fixes: #2563
mpkorstanje added a commit that referenced this pull request Jun 21, 2022
@mpkorstanje
[Core] Flush pretty output manually
2eef85d 
For the sake of efficiency and performance we write to buffered writers.
However, the pretty plugin is expected to print scenarios as they're started
and steps as they're finished.

This was initially resolved by flushing the `NiceAppendable` on every write.
This turned out to have performance impact (#2481) and interact poorly with
tools that expected (#2536) us to be better behaved.

By having `NiceAppendable` flush only when the underlying buffer was full these
problems were resolved (#2541) . However, because the underlying buffer is 8KiB
the pretty formatters output would effectively sit in a buffer until the writer
was closed.

So now we flush the pretty and progress output manually after each event. This
strikes a balance somewhere between the flush-everything-all-the-time and
flush-when-full approaches.

Fixes: #2563
mpkorstanje added a commit that referenced this pull request Jun 21, 2022
@mpkorstanje
[Core] Flush pretty output manually (#2573)
dc7a7d6 
For the sake of efficiency and performance we write to buffered writers.
However, the pretty plugin is expected to print scenarios as they're started
and steps as they're finished.

This was initially resolved by flushing the `NiceAppendable` on every write.
This turned out to have performance impact (#2481) and interact poorly with
tools that expected (#2536) us to be better behaved.

By having `NiceAppendable` flush only when the underlying buffer was full these
problems were resolved (#2541) . However, because the underlying buffer is 8KiB
the pretty formatters output would effectively sit in a buffer until the writer
was closed.

So now we flush the pretty and progress output manually after each event. This
strikes a balance somewhere between the flush-everything-all-the-time and
flush-when-full approaches.

Fixes: #2563
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpkorstanje mpkorstanje mpkorstanje left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pretty print plugin performance issues
3 participants
@scottadav @aslakhellesoy @mpkorstanje