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

Log widget improvements #8337

Open
wants to merge 6 commits into
base: master
from

Conversation

@CookiePLMonster
Copy link
Contributor

commented Aug 28, 2019

This PR largely overhauls LogWidget to drastically decrease its memory usage and negative impact on UI responsiveness.

In the worst case, spammy enough logs will cause LogWidget to lock up an entire Qt UI (as reported by @JMC47 and confirmed by my tests)). Additionally, both the widget itself and its log buffering functionality (designed so it never updates more than 200 lines in one go) were unbounded and could grow infinitely, taking GB's of RAM and maybe even causing Qt to run out of memory (I think that's why UI locked up eventually).

My changes include:

  • Most importantly, removal of QTextEdit in favour of QPlainTextEdit. The former is a full WYSIWYG editor with support for images, tables and rich text features we absolutely don't need in a log viewer! QPlainTextEdit is closer in functionality to Notepad with syntax highlighting (whereas QTextEdit could be compared to WordPad), and Qt documentation recommends using it for log viewers.
  • Removal of a synchronization mutex, because all calls to the buffer queue are done from the same thread.
  • Reconstruction of LogWidget::Log to make as little memory allocations and string copies as reasonably possible.
  • Removal of unused widgets (one of which was actually leaking) and class fields from LogWidget.
  • Addition of MAXIMUM_BLOCK_COUNT constant, currently set to 5000. Both log viewer widget and the log queue are now limited to only store this many most recent messages. Not only this drastically improves performance, but also prevents memory usage from exploding with spammy enough logs. If there is a need to preview more than 5000 recent log lines, please log to file or to a console.
  • Addition of additional constraints to widget updates. Now logs get appended to a Qt object only if widget is visible. This further improves UI responsiveness, making it completely fluid even with the most spammy logs, with hitches occuring only if Log tab is visible.
@Ebola16

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@BhaaLseN
Copy link
Member

left a comment

Changes seem sensible in general, but untested.

Source/Core/DolphinQt/Config/LogWidget.h Show resolved Hide resolved

@CookiePLMonster CookiePLMonster changed the title Log widget improvements [WIP] Log widget improvements Aug 29, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Idea: Since m_log_queue now has a fixed maximum size, I could replace it with a ring buffer (implemented on a std::vector for example) and remove any container allocations in the process, as it'd be preserved up front and never grown nor shrunk.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@Ebola16 @JMC47 I updated the PR with a removal of QueueOnObject calls. Logging workflow now uses a lock (again...) to put log lines in a buffer queue directly as opposed to queueing a Qt signals. I'm assuming those Qt signals had to use some synchronization internally anyway, and with sufficient log spam we were potentially sending thousands of those - so now it's done explicit by taking a lock and adding things to a queue, without clogging Qt signal queue.

UpdateLog is now move heavyweight, of course - but the upside is that std::string -> QStringconversions are now done **only** when log is actually displaying, whereas earlier they were done always, even if log lines were to be thrown away without being displayed ever. While I could perform this work without moving strings to a temporarystd::vector` before, I figured it makes sense to move them as then I can safely release the lock.

My profiling shows that this change improves logging performance further, but testing is appreciated.

There will be more coming (a ring buffer I mentioned earlier), so performance may still be in non-final state. Seeing how I already corrected two design decisions in consecutive commits, I think I'll rewrite history completely before removing the [WIP] tag.

Source/Core/DolphinQt/Config/LogWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Config/LogWidget.h Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Config/LogWidget.h Outdated Show resolved Hide resolved

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:log-widget-improvements branch 2 times, most recently from af291a7 to e045a93 Aug 30, 2019

@CookiePLMonster CookiePLMonster changed the title [WIP] Log widget improvements Log widget improvements Aug 30, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Commit history has been completely remade to make more sense and to remove intermediate experiments. According to git diff the end result is 1:1 identical with what was submitted before.

This is now ready for final reviews!

@Ebola16

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

I didn't do any quantitative analysis but the latest version of this PR seems to be even better! My test case was the workaround for https://bugs.dolphin-emu.org/issues/10791 (DSP LLE recompiler when using Project M Launcher and its derivatives). This tends to spam a TON of error messages with certain builds. After exiting emulation, the error message spam stops significantly sooner in this PR when compared to Master.

Function-wise LGTM but I don't feel comfortable enough with my coding skills to review the code itself.

LogWidget: Replace QTextEdit with QPlainTextEdit for better performance
QTextEdit is heavy, similar in functionality to WordPad,
while QPlainTextEdit is lightweight like Notepad.
Qt documentation recommends using QPlainTextEdit for log viewers,
and it also allows to set automatic cutoff of oldest messages beyond a fixed point,
which we now set to MAX_LOG_LINES (5000)
LogWidget: Stop update timer when log window is invisible so it doesn…
…'t continuously update in the background
LogWidget: Do not use QueueOnObject to construct log queue,
instead store a std::string constructed from string_view and convert to QString just before appending

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:log-widget-improvements branch from e045a93 to acefe27 Aug 31, 2019

Source/Core/DolphinQt/Config/LogWidget.h Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Config/LogWidget.h Outdated Show resolved Hide resolved

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:log-widget-improvements branch from acefe27 to 1cccdb1 Aug 31, 2019

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:log-widget-improvements branch 2 times, most recently from 48e03f3 to dc5692b Aug 31, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

circular_buffer is now obsolete, turns out we had a (slightly bugged) FixedSizeQueue for the same thing. The only thing it doesn't have compared to circular_buffer is construction behaviour - FixedSizeQueue holds default-initialized elements, while circular_buffer held unallocated buffers like STL.

FixedSizeQueue: Bugfixes and improvements
- Fixed a bug where pushing items over queue's size left it in a corrupted state
- For non-trivial types, have clear() and pop() run destructors
- Added emplace(args...)
- Added empty()

FixedSizeQueue has semantics of a circular buffer,
so pushing items continuously is expected to keep overwriting oldest elements gracefully.

Tests have been updated to verify correctness of a previously bugged behaviour
and to verify correctness of destructing non-trivial types
LogWidget: Use FixedSizeQueue for a log messages buffer
Messages buffer is intended to be of a fixed capacity (MAX_LOG_LINES),
which cannot be achieved by std::queue unless we manually pop() extra elements.
std::queue uses std::deque internally which most likely results in allocations performed continuously.
FixedSizeQueue keeps a single buffer during its entire lifetime, avoiding any allocations except the ones
performed by stored objects.

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:log-widget-improvements branch from dc5692b to 6bfa4fa Aug 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.