wxGenericProgressDialog gets slower with each update (Issue #26192)
dsa-t
dsa-t created an issue (wxWidgets/wxWidgets#26192)Description
- Apply this patch:
diff --git a/samples/dialogs/dialogs.cpp b/samples/dialogs/dialogs.cpp index a984f468fb..7a1faa4320 100644 --- a/samples/dialogs/dialogs.cpp +++ b/samples/dialogs/dialogs.cpp @@ -19,6 +19,7 @@ #include "../sample.xpm" +#include <chrono> #include "wx/apptrait.h" #include "wx/datetime.h" #include "wx/filename.h" @@ -3285,7 +3286,7 @@ void MyFrame::OnClose(wxCloseEvent& event) #if wxUSE_PROGRESSDLG -static const int max_ = 100; +static const int max_ = 10000000; void MyFrame::ShowProgress( wxCommandEvent& WXUNUSED(event) ) { @@ -3330,6 +3331,13 @@ void MyFrame::ShowProgressGeneric( wxCommandEvent& WXUNUSED(event) ) void MyFrame::DoShowProgress(wxGenericProgressDialog& dialog) { + + ::AllocConsole(); + + freopen("CONIN$", "r", stdin); + freopen("CONOUT$", "w", stdout); + freopen("CONOUT$", "w", stderr); + bool cont = true; for ( int i = 0; i <= max_; i++ ) { @@ -3337,7 +3345,8 @@ void MyFrame::DoShowProgress(wxGenericProgressDialog& dialog) // test both modes of wxProgressDialog behaviour: start in // indeterminate mode but switch to the determinate one later - const bool determinate = i > max_/2; + //const bool determinate = i > max_/2; + const bool determinate = true; if ( i == max_ ) { @@ -3363,6 +3372,9 @@ void MyFrame::DoShowProgress(wxGenericProgressDialog& dialog) } // will be set to true if "Skip" button was pressed + //::AttachConsole + auto start = std::chrono::high_resolution_clock::now(); + bool skip = false; if ( determinate ) { @@ -3373,6 +3385,11 @@ void MyFrame::DoShowProgress(wxGenericProgressDialog& dialog) cont = dialog.Pulse(msg, &skip); } + auto finish = std::chrono::high_resolution_clock::now(); + int ms = std::chrono::duration_cast<std::chrono::milliseconds>(finish - start).count(); + + wxPrintf("Update/Pulse took\t%d ms\n", ms); + // each skip will move progress about quarter forward if ( skip ) { @@ -3393,7 +3410,7 @@ void MyFrame::DoShowProgress(wxGenericProgressDialog& dialog) dialog.Resume(); } - wxMilliSleep(100); + //wxMilliSleep(10); } if ( !cont ) diff --git a/src/msw/evtloop.cpp b/src/msw/evtloop.cpp index c717dcdf26..6a52bd1cb7 100644 --- a/src/msw/evtloop.cpp +++ b/src/msw/evtloop.cpp @@ -377,6 +377,8 @@ void wxGUIEventLoop::DoYieldFor(long eventsToProcess) wxEventLoopBase::DoYieldFor(eventsToProcess); + wxPrintf( "msgsToProcess.size():\t\t%d\n", (int) msgsToProcess.size()); + // put back unprocessed events in the queue DWORD id = GetCurrentThreadId(); for ( const auto& m : msgsToProcess )
- Run "dialogs.exe" sample and see "Update/Pulse" times increase and the event queue getting bigger with each update:
Update/Pulse took 39 ms
msgsToProcess.size(): 4095
msgsToProcess.size(): 4095
Update/Pulse took 38 ms
msgsToProcess.size(): 4096
msgsToProcess.size(): 4099
Update/Pulse took 46 ms
msgsToProcess.size(): 4101
msgsToProcess.size(): 4101
Update/Pulse took 43 ms
msgsToProcess.size(): 4102
msgsToProcess.size(): 4102
Update/Pulse took 40 ms
msgsToProcess.size(): 4103
msgsToProcess.size(): 4104
Update/Pulse took 43 ms
msgsToProcess.size(): 4104
msgsToProcess.size(): 4106
Update/Pulse took 42 ms
msgsToProcess.size(): 4108
msgsToProcess.size(): 4109
Platform and version information
- wxWidgets version you use: master (3.3)
- wxWidgets port you use: wxMSW
- OS and its version: Windows 11
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26192)This happens only with generic progress dialog, right, not the native one?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26192)It looks like we're accumulating WM_TIMER messages, which is weird because I don't see any running timers. Still, we could just drop these messages and applying this seems to solve the particular problem here:
diff --git a/src/msw/evtloop.cpp b/src/msw/evtloop.cpp index c717dcdf26..94c8378ced 100644 --- a/src/msw/evtloop.cpp +++ b/src/msw/evtloop.cpp @@ -273,6 +273,7 @@ void wxGUIEventLoop::DoYieldFor(long eventsToProcess) // choose a wxEventCategory for this Windows message bool processNow; + bool processLater = true; switch (msg.message) { case WM_NCMOUSEMOVE: @@ -338,6 +339,14 @@ void wxGUIEventLoop::DoYieldFor(long eventsToProcess) case WM_TIMER: processNow = (eventsToProcess & wxEVT_CATEGORY_TIMER) != 0; + + // Timer messages will keep accumulating in the queue if we + // don't process them, so if we don't process this one, don't + // bother putting it back into the queue: this will lead to it + // growing with each call to this function, which is especially + // bad if it's called in a loop, as in wxGenericProgressDialog, + // and we'll get another timer message later anyhow. + processLater = false; break; default: @@ -369,9 +378,13 @@ void wxGUIEventLoop::DoYieldFor(long eventsToProcess) } else { - // remove the message and store it + // remove the message from the queue to ensure that we don't loop + // forever here in any case ::GetMessage(&msg, nullptr, 0, 0); - msgsToProcess.push_back(msg); + + // and perhaps save it for processing later + if ( processLater ) + msgsToProcess.push_back(msg); } }
Could you please check if this works in KiCad too? This is safer than unconditional wxYield() which can lead to unwanted/unexpected reentrancies with possibly fatal consequences.
But OTOH the whole approach is clearly flawed, when calling YieldFor() in a loop we end up processing each of not immediately dispatched events during every iteration, we need some API for collecting all unprocessed events outside of YieldFor() and then processing them only once at the end...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mark Roszko
marekr left a comment (wxWidgets/wxWidgets#26192)Hmm I hope there isn't interplay with other timers in kicad here. At one point our rendering loop was based on timers, but then @dsa-t changed it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dsa-t
dsa-t left a comment (wxWidgets/wxWidgets#26192)This happens only with generic progress dialog, right, not the native one?
I don't see such an issue with the native dialog.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26192)I think timers will continue to work just fine, they were not dispatched until the end of the progress dialog before (and I think this is correct, you wouldn't expect your timer callbacks being called while you're performing some long operation on the main thread) and still aren't, but once the dialog is closed they will keep firing — again, as before.
We could fine tune this by keeping a single copy of each unique WM_TIMER instead of discarding them entirely, but I don't think it's worth it. Please let me know if you actually see any problems with the simple fix above.
I don't see such an issue with the native dialog.
Yes, sorry, this was a stupid question, after looking at the code I've realized that the native dialog works in entirely different way.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26192)Thinking more about this, another possibility would be to apply this instead:
diff --git a/src/msw/evtloop.cpp b/src/msw/evtloop.cpp index c717dcdf26..0c30bac62c 100644 --- a/src/msw/evtloop.cpp +++ b/src/msw/evtloop.cpp @@ -337,7 +337,14 @@ void wxGUIEventLoop::DoYieldFor(long eventsToProcess) break; case WM_TIMER: - processNow = (eventsToProcess & wxEVT_CATEGORY_TIMER) != 0; + // We always process timer messages immediately, even if the + // caller didn't ask for them to be processed because they are + // synthesized by Windows if there are no other messages and so + // if we didn't handle them, they would keep accumulating in + // the queue and result in very noticeable slowdown if this + // function is called repeatedly in a loop, as it happens when + // wxGenericProgressDialog is used, for example. + processNow = true; break; default:
but this doesn't look great either, as calls to timer callbacks from inside progress dialog or some other code calling YieldFor(wxEVT_CATEGORY_UI) could be really unexpected.
On balance, I think it's still better to discard timer events than perform possibly unwanted callbacks.
But if you think that something could break in KiCad due to this, we could instead store these events until the end of the progress dialog and only dispatch them once it's about to be closed. This would require an API change, however.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.