QTimer::singleShot()'s syntax



  • Hi,

    I have recently updated the code of my project to use the new signal/slot syntax, and did this for my calls to QTimer::singleShot().

    Thus, I replaced something like:

    QTimer::singleShot(0, this, SLOT(callCheckSimulationResults()));
    

    with:

    QTimer::singleShot(0, this, &SimulationExperimentViewWidget::callCheckSimulationResults);
    

    Now, the problem with the new syntax is that I have found that my callCheckSimulationResults() method doesn't get called as often as it does using the old syntax. So, are the two syntaxes different or are they supposed to be exactly the same?...

    For a bit of context, you may want to check a GitHub issue that I created for my project. As for the code where that single shot is done, you can find it here.

    Thanks in advance for any help/suggestion,

    Alan


  • Qt Champions 2017

    Hi
    in regards to Qtimer sending the signal, there is absolutely zero
    difference between the 2 syntax's.

    So its something else if your slot is called less often than before. (IMHO)

    The new syntax provides compile time check of the connection and allows to connects to lambdas/callables. Its recommended to use. The only downside to new syntax is
    eye hurting ugly casts if the signals are overloaded.



  • From https://github.com/opencor/opencor/blob/55718f183f498cf21793897f5772d52144c2350f/src/plugins/simulation/SimulationExperimentView/src/simulationexperimentviewwidget.cpp#L543

    we cannot ask QTimer::singleShot() to call checkSimulationResults() directly since it expects a file name

    You can with Qt5 and this is the root of your problem. checkSimulationResults always uses the first() result but there's nothing assuring you every results is first() at least once.

    • delete callCheckSimulationResults completely
    • Replace QTimer::singleShot(0, this, &SimulationExperimentViewWidget::callCheckSimulationResults);
      with QTimer::singleShot(0, this, std::bind(&SimulationExperimentViewWidget::checkSimulationResults,this,pFileName,SimulationExperimentViewSimulationWidget::None));


  • @mrjj: I am more than happy to accept that there may be something else that is "wrong" in my code. However, all I did in my code was replace one syntax with another. I didn't change anything else, just the call to the single shot, as mentioned in my original message. Yet, the behaviour I am getting is clearly different (and definitely reproducible). So, why?

    @VRonin: thanks, I have just given it a try and I can indeed confirm that it works, but I am getting the same behaviour than I am getting when using the new signal/slot syntax. In other words, I am not getting the same behaviour that I am getting using the old syntax. So, it doesn't seem to be the root of my problem.


  • Moderators

    @agarny said in QTimer::singleShot()'s syntax:

    Now, the problem with the new syntax is that I have found that my callCheckSimulationResults() method doesn't get called as often as it does using the old syntax.

    What do you mean by "doesn't get called as often"? How often are you firing single-shot timers?

    Place a qDebug() call just before each call to QTimer::singleShot() and run your code to see if that reveals any clues. Do this using both the new and old syntaxes.



  • @JKSH: I mean that, using the old syntax, my slot might get called 100 times while, using the new syntax, it might only get called 80 times. FWIW, I use that single slot mechanism to output simulation data that is being generated on the fly. In other words, my slot checks whether new simulation data is available and, if so, plots it.

    Using the old syntax, everything gets plotted:

    alt text

    while using the new syntax, only some of my simulation data gets plotted:

    alt text

    Just look at the blue traces at the top. The beginning of those traces is plotted, but the end of them is not always plotted, and that is a direct consequence of the new syntax.

    Regarding your suggestion, it's not that simple since the frequency of those calls to QTimer::singleShot() depends on how busy my computer is when running a simulation. All I can say is that using the old syntax, all my traces are full while, using the new syntax, some of them are full some not.



  • @agarny mmh,
    have you tried this with QTimer as a class member?
    I would imagin that creating a QTimer-object a 100 times per second is a heavy task, and comes with a cost.


  • Moderators

    @agarny said in QTimer::singleShot()'s syntax:

    Just look at the blue traces at the top. The beginning of those traces is plotted, but the end of them is not always plotted, and that is a direct consequence of the new syntax.

    Understood. The traces show symptoms of the pathology, but it reveals very little about the mechanism of the pathology.

    We expect both syntaxes to perform exactly the same functions. We don't expect them to behave exactly the same under the hood though, because the old syntax involves searching a list of strings to find your slot, whereas the new syntax simply stores a function pointer to your slot. I expect the new syntax to be marginally more efficient so your computer should be marginally less "busy" during the simulation.

    Some possible mechanisms I can think of (without reverse-engineering your whole project) are:

    1. There's a bug in QTimer.
      • It sometimes fails to trigger as instructed.
    2. There's a race condition in your code.
      • When you switched to the new syntax, its different behaviour under-the-hood caused the timings of different parts of your program to change relative to each other, which broke an assumption about execution order. Consequently, your code sometimes doesn't initialize a new single-shot timer when it should.
      • Perhaps your code no longer gives the file enough time to be finalized before trying to read it again?

    Regarding your suggestion, it's not that simple since the frequency of those calls to QTimer::singleShot() depends on how busy my computer is when running a simulation.

    Then do your best to make your computer's busy-ness as uniform as possible during different trials -- just close all other programs and run this one only. Like in cell biology, it doesn't matter if each trial isn't exactly the same. As long as their environments are similar enough, they will still let you identify trends/patterns.

    It's straightforward to check if mechanism #1 is responsible or not: Add qDebug("PING"); just before QTimer::singleShot() and add qDebug("\tPONG"); to the top of callCheckSimulationResults().

    • If the number of PINGs is greater than the number of PONGs, that strongly suggests a bug in QTimer.
    • If there are equal numbers of PINGs and PONGs, yet you still see truncated waveforms, then I'd suspect a bug in your code.


  • @J.Hilk: do you mean something like:

    static QTimer timer;
    
    timer.singleShot(0, this, &SimulationExperimentViewWidget::callCheckSimulationResults);
    

    If so, I have just given it a try, but to no avail (some traces are still incomplete) while, as you would, expect:

    static QTimer timer;
    timer.singleShot(0, this, SLOT(callCheckSimulationResults()));
    

    works fine.



  • @JKSH: the data I am plotting doesn't come from a file, but is computed and then stored in memory. So, all my program does (through that single shot) is check whether new data is available and, if so, plots it.

    Regarding your suggestion, it's not that simple since the frequency of those calls to QTimer::singleShot() depends on how busy my computer is when running a simulation.

    Then do your best to make your computer's busy-ness as uniform as possible during different trials -- just close all other programs and run this one only. Like in cell biology, it doesn't matter if each trial isn't exactly the same. As long as their environments are similar enough, they will still let you identify trends/patterns.

    Actually, I should take that comment back since my computer's CPU load is not that different when trying the old or new syntax.

    It's straightforward to check if mechanism #1 is responsible or not: Add qDebug("PING"); just before QTimer::singleShot() and add qDebug("\tPONG"); to the top of callCheckSimulationResults().

    • If the number of PINGs is greater than the number of PONGs, that strongly suggests a bug in QTimer.
    • If there are equal numbers of PINGs and PONGs, yet you still see truncated waveforms, then I'd suspect a bug in your code.

    I just had a quick go at it and the number of PINGs and PONGs is the same whether I use the old or new syntax. So, QTimer::singleShot() works as expected. However, there are more PINGs/PONGs using the old syntax than the new one, and I am not sure why yet...



  • @agarny

    Not quite what I had in mind.
    I meant something like this:

    //.h
    QTimer myTimer;
    
    //.cpp constructor
    {
       myTimer.setSingleShot(true);
       myTimer.setInterval(0); 
       connect(&myTimer, &QTimer::timeout, &SimulationExperimentViewWidget::callCheckSimulationResults);
    }
    

    and later in your code:

    myTimer.start();
    

    edit:

    actually, you could also try a connect via MetaObject::invokeMethod, see how that behaves:

    #if QT_VERSION >= 0x50a00
    QMetaObject::invokeMethod(this, &SimulationExperimentViewWidget::callCheckSimulationResults,Qt::QueuedConnection);
    #else
    QMetaObject::invokeMethod(this,"callCheckSimulationResults",Qt::QueuedConnection);
    #endif
    


  • @J.Hilk: ok, I have given your QTimer as a class member suggestion a go and some traces are incomplete, both using the old or new connect syntax, although it's (still) worse using the new syntax.

    Well, I am using Qt 5.9.4, so I am not able to test

    QMetaObject::invokeMethod(this, &SimulationExperimentViewWidget::callCheckSimulationResults, Qt::QueuedConnection);
    

    but

    QMetaObject::invokeMethod(this, &SimulationExperimentViewWidget::callCheckSimulationResults, Qt::QueuedConnection);
    

    works as expected, but I imagine that it's not surprising. It's the other syntax that would be interesting to check.


  • Moderators

    @agarny said in QTimer::singleShot()'s syntax:

    I have given your QTimer as a class member suggestion a go and some traces are incomplete, both using the old or new connect syntax, although it's (still) worse using the new syntax.

    • Your original code has the most overhead.
    • The member-QTimer with old syntax has moderate overhead.
    • The member-QTimer with new syntax has the least overhead.

    Greater overhead means greater delays in triggering the slot, and greater delays seem to result in greater reliability in your plots. This strongly suggests to me that your code has a race condition.

    If this is the case, then simply upgrading to a more powerful computer can give you incomplete traces too, even with your original code with old syntax.

    @agarny said in QTimer::singleShot()'s syntax:

    I just had a quick go at it and the number of PINGs and PONGs is the same whether I use the old or new syntax. So, QTimer::singleShot() works as expected. However, there are more PINGs/PONGs using the old syntax than the new one, and I am not sure why yet...

    With the PING/PONG test in place, did you see any truncated waveforms?

    Next, I'd investigate to see why fewer PINGs are being generated. I'm guessing your last if() is not entering the right case, so I'd probe that:

    if (simulation->isRunning() || (simulationResultsSize != simulation->results()->size())) {
        mSimulationCheckResults << pFileName;
        qDebug() << "PING" << pFileName;
        QTimer::singleShot(0, this, &SimulationExperimentViewWidget::callCheckSimulationResults);
    } else if (!simulation->isRunning() && !simulation->isPaused()) {
        qDebug("\tEND");
        mSimulationResultsSizes.remove(pFileName);
        simulationWidget->resetSimulationProgress();
    } else {
        qDebug("\t???");
    }
    

    Other things that might be worth investigating:

    • Log the values of simulationResultsSize and mSimulationResultsSizes.value(pFileName) every call. Look for differences in values when you switch syntaxes.
    • Log the times of every call, using QTime::currentTime() or QElapsedTimer::elapsed() (the latter needs a member object). Look for differences in values when you switch syntaxes.


  • @agarny said in QTimer::singleShot()'s syntax:

    I have recently updated the code of my project to use the new signal/slot syntax, and did this for my calls to QTimer::singleShot().

    Just to absolutely clear (although you do seem to know what you are talking about, so this is perhaps obvious to you), given you are making this change:

    • You're not doing the change against a different version/compilation of Qt, are you?
    • Given that you are "cleaning up" code, there aren't any other changes to your code, are there?


  • @JKSH said in QTimer::singleShot()'s syntax:

    @agarny said in QTimer::singleShot()'s syntax:

    I have given your QTimer as a class member suggestion a go and some traces are incomplete, both using the old or new connect syntax, although it's (still) worse using the new syntax.

    • Your original code has the most overhead.
    • The member-QTimer with old syntax has moderate overhead.
    • The member-QTimer with new syntax has the least overhead.

    Greater overhead means greater delays in triggering the slot, and greater delays seem to result in greater reliability in your plots. This strongly suggests to me that your code has a race condition.

    Yes, I have just done some tests timing the interval with which my single shots are called and the intervals are greater with the old syntax.

    If this is the case, then simply upgrading to a more powerful computer can give you incomplete traces too, even with your original code with old syntax.

    I am happy to believe that, now that I have got those intervals.

    With the PING/PONG test in place, did you see any truncated waveforms?

    Yes, I did. In fact, looking at the plots I got, I couldn't tell the difference with or without the PING/PONG test.



  • @JonB said in QTimer::singleShot()'s syntax:

    @agarny said in QTimer::singleShot()'s syntax:

    I have recently updated the code of my project to use the new signal/slot syntax, and did this for my calls to QTimer::singleShot().

    Just to absolutely clear (although you do seem to know what you are talking about, so this is perhaps obvious to you), given you are making this change:

    • You're not doing the change against a different version/compilation of Qt, are you?
    • Given that you are "cleaning up" code, there aren't any other changes to your code, are there?

    Indeed, I am using the same version of Qt (5.9.4) with both the old and new syntax. In fact, when it comes to this thread, the only thing I have done to my code is using either:

    QTimer::singleShot(0, this, SLOT(callCheckSimulationResults()));
    

    or

    QTimer::singleShot(0, this, &SimulationExperimentViewWidget::callCheckSimulationResults);
    

    The rest of my code is exactly the same.


  • Qt Champions 2017

    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L199

    Your threading is all jumbled up, you can't just read and modify fields from objects that are in different threads ... if it works, it's the hand of god, if it doesn't work, it's the hand of god as well ... (and that one comes from an atheist)

    To give you a straightforwardly obvious example:
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L95

    What's guarding mCurrentPoint?
    I would also imagine you may get a lot of runtime warnings from Qt about objects being created before QApplication (due to singletons) and possibly also warnings about destruction order or destructions from different threads.



  • @kshegunov: yes, I think it's starting to become very clear that I am facing a race condition and that I have been pretty lucky these past few years. Ok, I guess I am going to have to check that very carefully. (In the end, it's a good thing that I decided to use the new syntax... (Positive thinking... :))



  • @kshegunov: by the way, no, I am not getting any runtime warnings from Qt.


  • Qt Champions 2017

    a race condition

    More than one, I noticed at least several in that particular file.

    by the way, no, I am not getting any runtime warnings from Qt.

    I said you may, not that you are going to, as I didn't check the whole source. I spotted a place or two where you use the singleton, so that's usually accompanied by such warnings.



  • @kshegunov said in QTimer::singleShot()'s syntax:

    a race condition

    More than one, I noticed at least several in that particular file.

    Would you mind letting me know (via MP, if you want) those you noticed?

    by the way, no, I am not getting any runtime warnings from Qt.

    I said you may, not that you are going to, as I didn't check the whole source. I spotted a place or two where you use the singleton, so that's usually accompanied by such warnings.

    Yes, I know what you said. I was merely pointing out that, in my particular case, I am not getting runtime warnings.

    Otherwise, when it comes to my singletons, I believe they are always used in the main thread. This being said, I am going to double check and also make sure that they are thread safe. So, thanks for the reminder.


  • Qt Champions 2017

    @agarny said in QTimer::singleShot()'s syntax:

    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L207
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L214
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L215
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L217
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L235
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L236
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L237
    https://github.com/opencor/opencor/blob/master/src/plugins/support/SimulationSupport/src/simulationworker.cpp#L244

    And so on ... the list is very, very long.

    As far as I could see only the SimulationWorker object is moved to a separate thread, so you should focus your efforts there. For one don't pass it objects that are in different threads (e.g. Simulation class's mSimulation), as it gets quite alluring to just call methods on them (which is a race condition). Instead, move the setters of SimulationWorker as slots and emit signals when any of the private/calculated data has changed. Then you can connect those signals and slots from the outside and Qt will take care of the access serialization for you.



  • @kshegunov: thanks, I appreciate the heads-up.

    FWIW, the purpose of my SimulationWorker class is, as you probably gathered, to compute some mathematical model, so I don't want this work to overload the main thread (and therefore "freeze" my GUI), hence I indeed decided to move it to a separate thread.

    Regarding the Simulation object I pass to my SimulationWorker object, it is used to retrieve some information about my simulation (which have been set before running the worker and that don't get changed during the worker's lifespan), as well as add to it the results of the worker (i.e. simulation results). Then, my main thread (and the reason I originally decided to have that QTimer::singleSlot() calls) checks whether new results are available. So, no writing there, just fetching.

    Anyway, I appreciate that it could be improved when it comes thread safety, and I will make sure that it is. FWIW, I did, at some point, rely on the signal/slot mechanism for my worker, but it slowed things down quite a bit, hence I went for the approach I have just described above.


  • Moderators

    @agarny said in QTimer::singleShot()'s syntax:

    Anyway, I appreciate that it could be improved when it comes thread safety, and I will make sure that it is. FWIW, I did, at some point, rely on the signal/slot mechanism for my worker, but it slowed things down quite a bit, hence I went for the approach I have just described above.

    See http://doc.qt.io/qt-5/threads-synchronizing.html It sounds like the high-level approach slows things down noticeably for you, so go with the low-level ones instead.



  • @JKSH said in QTimer::singleShot()'s syntax:

    See http://doc.qt.io/qt-5/threads-synchronizing.html It sounds like the high-level approach slows things down noticeably for you, so go with the low-level ones instead.

    Yes, I am already using low-level synchronization primitives in parts of my code, and this is the approach I am thinking of taking here too.



  • Ok, on further investigation, the symptom described in my original message is not the result of a race condition. I agree that it is not to say that my code is free of race conditions, although I am relatively confident that it is. Not so much because of the quality of my code, but rather because of the way it is used. Now, I know it's not great, but that's the compromise I had to make for my code to be as fast as possible, something that is important for my application.

    Anyway, among other things, my application can "run" mathematical models and, until recently, it could only render one run at a time. Recently, I have modified my application so that it could render multiple runs and this is where the problem was. I wasn't properly handling multiple runs and because of the low overhead associated with the new QTimer::singleShot syntax (compared with the old syntax), my application didn't render the end of some runs. I now "properly" handle multiple runs and everything works as expected (see here for those who had a look at my code before).

    So, thanks all for your feedback, it was much appreciated. (Thanks @VRonin for your std::bind() suggestion, which I now use.)


  • Qt Champions 2017

    @agarny said in QTimer::singleShot()'s syntax:

    Now, I know it's not great, but that's the compromise I had to make for my code to be as fast as possible, something that is important for my application.

    Are you really going to say that fast code must be buggy?

    Good luck with newer versions of your project. It may work more or less now, but when you add functionality sooner or later all uncleanliness hits back.



  • Mathematical model + race condition = quantum model I guess 🙂



  • @aha_1980 said in QTimer::singleShot()'s syntax:

    @agarny said in QTimer::singleShot()'s syntax:

    Now, I know it's not great, but that's the compromise I had to make for my code to be as fast as possible, something that is important for my application.

    Are you really going to say that fast code must be buggy?

    No, this is not what I am going to say otherwise I would have already said it.

    I merely said that to get the speed I was after I felt the need to make some compromises and that I am aware of their limitations, hence I have done my best to ensure that my code doesn't result in a race condition, etc. Should I ever find a solution that is "cleaner" and as fast as my current solution, then I will clearly implement it.

    (Thanks for the sarcasm.)


  • Qt Champions 2017

    @aha_1980's point is that you're building a house of cards here. Even if you don't see the effects now it is going to bite you in the a$$ sometime along the road. Remember what I wrote before: "If it works, it's the hand of god", well that's what he and I both mean. It's your code in the end and you can conduct your business as you like, we just felt it's a rather weak argument to ignore the simmering reality because you couldn't get enough speed at the time. In the end the speed would not matter if you don't get correct results, would it?



  • @kshegunov: I do hear what you are both saying, believe it or not (see above my comment about ever finding a "cleaner" solution).


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.