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

Fix #124 (slow prints when data is sent in chunks) #125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix #124 (slow prints when data is sent in chunks) #125

wants to merge 2 commits into from

Conversation

neon-dev
Copy link
Contributor

@neon-dev neon-dev commented Sep 9, 2021

Should fix #124 but untested.
Maybe ReadLongRunningTask() should get the same treatment, not sure.

@neon-dev
Copy link
Contributor Author

neon-dev commented Sep 9, 2021

Hmm, after some googling a BlockingCollection should be the better solution as it works without arbitrary delays.
I'm not very experienced in C#, so sorry this fix isn't ideal.

@lukevp
Copy link
Owner

lukevp commented Sep 16, 2021

I haven't reviewed yet but I hope to do so this weekend. In the meantime, @neon-dev would you mind adding a new test type to the console app as part of this PR that reproduces your issue? If I understand correctly, the issue is because there's a long receipt with each line being a separate printer.Write() call instead of using a ByteSplicer.Combine() to send a single write call? Would be good to have an example in the console test suite since we typically run through those before publishing a new version and would make sure we don't have a performance regression here in the future.

@neon-dev
Copy link
Contributor Author

neon-dev commented Sep 16, 2021

I counted how many individual Write calls the shortest possible receipt takes (including commands for alignment etc.) and got 45.
So the old synchronous code can be tested with these simple lines and results in ~31 ms:

var stopwatch = new Stopwatch();
stopwatch.Start();
for (int i = 0; i < 45; i++) {
    printer.Write(E.LeftAlign());
}
stopwatch.Stop();
Console.WriteLine($"Duration: {stopwatch.ElapsedMilliseconds} ms");

But the new asynchronous code will report 0 ms and can only be measured by modifying the BasePrinter class and adding a(nother) wait logic somewhere to detect when the WriteBuffer is fully drained.
I don't know about you but I dislike this idea.

We wouldn't have to worry about regressions if the async task would use said BlockingCollection backed by a ConcurrentQueue because then there are no delays involved, which would make everything even more responsive while also removing the potential for CPU load issues.
Unfortunately I won't find enough time in the near future to update my PR with this superior, alternative solution.

@neon-dev
Copy link
Contributor Author

neon-dev commented Jul 31, 2022

@lukevp have you looked into a proper solution?
I'd also suggest a note about this performance issue in the README in the meantime.

@neon-dev neon-dev closed this by deleting the head repository Feb 26, 2023
@neon-dev neon-dev reopened this Feb 26, 2023
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.

Slow print after upgrade
2 participants