allocate short-living widgets on the heap or the stack?

Justin Sayne

What is the usual way to handle memory management issues arising in this use-case:

void MainWidget::handle_btn_press() {
  SomeDialog* dlg = new SomeDialog(this);
  auto params = dlg->get_some_params();
  Algorithm::run_some_algo(params);
}

Since Qt provides memory management for parented widgets and the internal object data ends up on the heap anyway (PIMPL), it doesn't seem to make much sense allocating widgets on the stack. However, assuming the slot above is executed a whole lot of times during a typical application lifetime, doesn't this clutter my MainWidget with more and more non-referenced SomeDialogs as widget children? Do I have to manually call deleteLater now to free up resources from non-used dialogs when they aren't used anymore and not just at the end of MainWidget's lifetime?
Another option would be to make the SomeDialog pointer a class member and just allocate a new dialog once. However I don't like introducing unnecessary (class-)global state when I really only use the object locally.

Intrigued to get some clarification on this :)

horokey

If your dialog is modal, as it looks like from your example, it'll be better to create it on the stack, as it is much more safer and faster way.

Rondog

For stuff like this I would put it on the heap but only create one instance of it. Something like this:

void MainWidget::handle_btn_press() 
{
  if(!d_my_dlg)
  {
    d_my_dlg = new SomeDialog(this);
  }

  d_my_dlg->Reset(); // make it look unused

  auto params = dlg->get_some_params();
  Algorithm::run_some_algo(params);
}

The instance of SomeDialog is initialized to zero (null) and possibly may never be created if it is never used. If it is used it is created just once.

kshegunov

@Justin-Sayne
Hi,

it doesn't seem to make much sense allocating widgets on the stack.

So your argument is that two heap allocations are better than one? While this might hold for apples, sadly a heap allocation costs, especially if you compare it with a stack-based one. There's a whole part of the OS that deals only with how and where to "put" your heap allocated objects (i.e. everything you create with new) - the heap manager; while a stack allocation boils down to a single addition (that is incrementing the stack pointer register) and nothing more. So since you already seem to realize Qt uses PIMPL to keep binary compatible, then you should also realize the QDialog object is the size of void *. Creating a QDialog on the heap then means you're creating an integer in the heap and that integer (actually a pointer) will reference the real data in another place in memory altogether. Would you generally do

int * a = new int;
*a = 8
delete a;

if you had another way? I wouldn't.

However, assuming the slot above is executed a whole lot of times during a typical application lifetime, doesn't this clutter my MainWidget with more and more non-referenced SomeDialogs as widget children?

It does.

Do I have to manually call deleteLater now to free up resources from non-used dialogs when they aren't used anymore and not just at the end of MainWidget's lifetime?

You don't have to, but you should to keep your program lean. Otherwise there's a ton of dialogs just hanging in memory waiting for the parent to get deleted.

Another option would be to make the SomeDialog pointer a class member and just allocate a new dialog once.

What about making SomeDialog a member and not allocating with new altogether?

class MainWidget : public QWidget
{
    Q_OBJECT
public:
    MainWidget(QWidget * = nullptr);

private slots:
    void handle_btn_press();

private:
    SomeDialog dlg;
};

MainWidget::MainWidget(QWidget * parent)
    : QWidget(parent), dlg(this)
{
}

void MainWidget::handle_btn_press()
{
    dlg.exec();
    // Do whatever is you do after the dialog has returned
}

However I don't like introducing unnecessary (class-)global state when I really only use the object locally.

Then just create and use it locally:

void MainWidget::handle_btn_press()
{
    SomeDialog dlg;
    dlg.exec();
    // Do whatever is you do after the dialog has returned
}

SGaist

Hi,

To add to @kshegunov, there's also a memory leak here. While the dialogs will be deleted when MainWidget gets destroyed, in between, each time handle_bth_press is called, a new instance of the dialog is created and then "lost".

Justin Sayne

@SGaist yes, that's what I meant. Imo it's not really a leak since the parent has taken ownership and still lives, but yeah, technically I don't need the dialog anymore once I've left method scope. Based on @kshegunov 's explanations, I will just go with the stack-based approach then. ;)

SGaist

I meant it's a leak in the sense that until MainWidget gets destroyed you are going to increase memory usage each time you call handle_btn_press.

Chris Kawa

Here's another approach for "short lived" non-modal dialogs and a rare example of when singleton pattern actually makes sense. We only keep one dialog instance around for as long as it is visible and the cost when we don't use it is just one smart pointer.

void MainWidget::handle_btn_press()
{
    static QPointer<SomeDialog> dlg;
    if (!dlg) {
        dlg = new SomeDialog();
        dlg->setAttribute(Qt::WA_DeleteOnClose);
    }
    dlg->show();
}

For modal dialogs I'm with @kshegunov. Just do this:

void MainWidget::handle_btn_press()
{
    SomeDialog dlg(this); //parents are important not only for lifetime management!
    dlg.exec();
}

I wouldn't make the dialog a class member just because it can be heavy and unnecessary to keep in memory all the time.

The debate of whether to put the dialog on a stack or heap is imo pointless. Even if creating it on the heap is somewhat wasteful the overhead is next to nothing compared to the weight of creating and showing a full blown window. I'd say keep it sane - create it on the stack if you can but don't force it if it's inconvenient for other reasons. Performance matters but premature micro-optimizations can greatly hinder readability and become a maintenance burden.

kshegunov

@Chris-Kawa

Here's another approach for "short lived" non-modal dialogs and a rare example of when singleton pattern actually makes sense. We only keep one dialog instance around for as long as it is visible and the cost when we don't use it is just one smart pointer.

It's a valid approach, but I'd always prefer connecting a signal from the dialog (or a dialog's button) to deleteLater() and/or any other slot to respond to the dialog closing. It's a matter of preference in this case, I just don't see a great gain in keeping a smart pointer reference to the dialog.

(As for your note about the parent - it's a valid observation, and my example has a typo, indeed. I've forgotten to set it up as in the class member example where I it's done in the constructor initializer list)

Even if creating it on the heap is somewhat wasteful the overhead is next to nothing compared to the weight of creating and showing a full blown window.

True, although I think there's no window creation and/or showing done when the constructor is run, am I in error?

I'd say keep it sane - create it on the stack if you can but don't force it if it's inconvenient for other reasons.

I completely agree!

Performance matters but premature micro-optimizations can greatly hinder readability and become a maintenance burden.

I don't consider replacing trivial heap allocations like:

QDialog * dlg = new QDialog(this);
dlg->exec();
delete dlg;

with trivial stack allocations like:

QDialog dlg(this);
dlg.exec();

premature or hindering readability, nor burdening maintenance. I'll grant you, though, most of the time one will not see any difference performance-wise.

Kind regards.

Chris Kawa

@kshegunov said:

I just don't see a great gain in keeping a smart pointer reference to the dialog.

This is to ensure there's only one instance of it and AFAIK there is no closing() signal in a dialog? Sure you could do something like this instead:

static SomeDialog* dlg = nullptr;
if (!dlg) {
    dlg = new SomeDialog();
    connect(dlg, &SomeDialog::someCustomClosingSignal, [&]
    {
        dlg->deleteLater();
        dlg = nullptr;
    });
}

It's totally valid but it's just more typing and extra dialog code (that other code might not know about). I think it's also easier to sneak a bug in code like this (more code -> more bugs ;) )

With all other points I totally agree. My optimization comment was a little out of place I guess. I have a tendency to further the discussion far ahead in my head and not to put all of it in writing, which kinda makes it look like I'm criticizing everything. Sorry about that. There's a lot of great points in this topic and ,as said in my previous entry, I completely agree about the stack based approach for modals - it's basically a win-win-win - faster, less code and hard to get wrong.

kshegunov

@Chris-Kawa

This is to ensure there's only one instance of it

Ah, yes, I completely slept through this minor detail ... :$

Sure you could do something like this instead

No thanks, I'll take your QPointer suggestion any day. The latter is ... well it's work-aroundishly ugly ...

Sorry about that.

Not at all, I agree with the "premature optimization is the root of all evil" stance I just didn't see the proximate cause for it here, that's all.

... hard to get wrong.

This is what I always liked about the stack, as I'm lazy and don't like debugging bad code ... especially if it's mine ... ;)