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

Closing instrument dialogs doesn't delete underlying object #420

Closed
nyanpasu64 opened this issue Sep 15, 2021 · 2 comments
Closed

Closing instrument dialogs doesn't delete underlying object #420

nyanpasu64 opened this issue Sep 15, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@nyanpasu64
Copy link
Contributor

Checklist

  • I am reporting exactly 1 bug with this issue.
  • This bug hasn't already been reported.
  • This bug hasn't already been fixed in the latest development build.

Bug Description

In #412, I noted that InstrumentFormManager holds a map of std::shared_ptr<QWidget/QDialog>. I don't think this is a good idea (the QDialogs survive being closed and are only destroyed when you delete the instrument or open a new document).

I checked with htop - creating 128 new instruments & opening their forms - and I see what you mean.

Does that mean they take up RAM? I can't confirm. When I tested opening and closing many dialogs, I found that the more dialogs I had opened and closed (but their objects still hang around), the longer each new dialog takes to open. (Previously opened dialogs are still instant to open). Additionally, all future actions (like opening file dialogs, About, trying to close the window) slow down as well.

gdb and perf (perf record --call-graph fp ... + Hotspot) revealed that Qt was spending time in QCoreApplicationPrivate::sendThroughApplicationEventFilters, as if every dialog had installed its own application-wide event filter. I found that this is due to the use of one SliderStyle : QProxyStyle per labeled slider. Perhaps a single SliderStyle should be shared? (though it may be illegal to make a static variable QProxyStyle before QApplication is initialized.) Maybe LabeledVerticalSlider's constructor should take a mandatory SliderStyle argument. (It would break using it in Qt Designer, but it doesn't matter because Qt Designer already doesn't create an actual LabeledVerticalSlider widget, just an empty box.)

I'm not sure if fixing the dialog leak is sufficient, but I think we should fix the SliderStyle leak as well (#418).

How to reproduce

  1. In an empty document, create 128 instruments, eg. using an auto mouse clicking tool. Alternatively download and open 32 64 128 instruments.zip.
  2. Start opening and closing instruments one by one.

System Information

  • Operating System: Arch Linux, KDE Plasma 5.22.5
  • BambooTracker Version: unstable-a468ee62
  • Build Type: AUR bambootracker-git
@nyanpasu64 nyanpasu64 added the bug Something isn't working label Sep 15, 2021
@rerrahkr
Copy link
Member

I apologize for not responding to your report for so long.
I had implemented it this way because I wanted to reduce the delay in reopening a dialog once it had been opened.
However, now that I have tried it, the delay is so small that I do not notice it when I create a new dialog each time. I will consider changing it, as you are right, it is a waste of memory.

@rerrahkr
Copy link
Member

rerrahkr commented Jun 8, 2022

I modified InstrumentEditorManager (renamed from InstrumentFormManager) to free an instrument editor when it is closed at fac5975.

@rerrahkr rerrahkr closed this as completed Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants