From e71e4c932a4e9e77207f968b150b51d5009408b5 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 16 Apr 2011 17:27:30 +0000 Subject: [PATCH] Don't block the main UI thread while generating completions in wxMSW. The native IAutoComplete implementation takes care to retrieve the completions from a background thread to prevent the UI from freezing while they're being generated, but we worked against it by always generating all the completions from the main thread and just enumerating them from the background one. Change this now and call wxTextCompleter::GetCompletions() method from the background thread itself to never block the main one. git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@67514 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775 --- interface/wx/textcompleter.h | 4 + src/msw/textentry.cpp | 164 ++++++++++++++++++++++++++++++++--- 2 files changed, 156 insertions(+), 12 deletions(-) diff --git a/interface/wx/textcompleter.h b/interface/wx/textcompleter.h index c900821379..275716c00d 100644 --- a/interface/wx/textcompleter.h +++ b/interface/wx/textcompleter.h @@ -74,6 +74,10 @@ public: otherwise they will be simply ignored, making adding them to the array in the first place useless. + Notice that this function may be called from thread other than main one + (this is currently always the case under MSW) so care should be taken + if it needs to access any shared data. + @param prefix The possibly empty prefix that the user had already entered. @param res diff --git a/src/msw/textentry.cpp b/src/msw/textentry.cpp index f2d8eafd56..d0bde2fad6 100644 --- a/src/msw/textentry.cpp +++ b/src/msw/textentry.cpp @@ -43,7 +43,7 @@ #define GetEditHwnd() ((HWND)(GetEditHWND())) // ---------------------------------------------------------------------------- -// wxIEnumString implements IEnumString interface +// Classes used by auto-completion implementation. // ---------------------------------------------------------------------------- // standard VC6 SDK (WINVER == 0x0400) does not know about IAutoComplete @@ -73,11 +73,45 @@ DEFINE_GUID(CLSID_AutoComplete, 0x00bb2763, 0x6a77, 0x11d0, 0xa5, 0x35, 0x00, 0xc0, 0x4f, 0xd7, 0xd0, 0x62); +namespace +{ + +// Small helper class which can be used to ensure thread safety even when +// wxUSE_THREADS==0 (and hence wxCriticalSection does nothing). +class CSLock +{ +public: + CSLock(CRITICAL_SECTION& cs) : m_cs(&cs) + { + ::EnterCriticalSection(m_cs); + } + + ~CSLock() + { + ::LeaveCriticalSection(m_cs); + } + +private: + CRITICAL_SECTION * const m_cs; + + wxDECLARE_NO_COPY_CLASS(CSLock); +}; + +} // anonymity namespace + +// Implementation of string enumerator used by wxTextAutoCompleteData. This +// class simply iterates over its strings except that it also can be told to +// update them from the custom completer if one is used. +// +// Notice that Next() method of this class is called by IAutoComplete +// background thread and so we must care about thread safety here. class wxIEnumString : public IEnumString { public: wxIEnumString() { + Init(); + m_index = 0; } @@ -85,9 +119,29 @@ public: : m_strings(other.m_strings), m_index(other.m_index) { + Init(); } - DECLARE_IUNKNOWN_METHODS; + void ChangeStrings(const wxArrayString& strings) + { + CSLock lock(m_csStrings); + + m_strings = strings; + m_index = 0; + } + + void UpdateStringsFromCompleter(wxTextCompleter *completer, + const wxString& prefix) + { + CSLock lock(m_csUpdate); + + // We simply store the pointer here and will really update during the + // next call to our Next() method as we want to call GetCompletions() + // from the worker thread to prevent the main UI thread from blocking + // while the completions are generated. + m_completer = completer; + m_prefix = prefix; + } virtual HRESULT STDMETHODCALLTYPE Next(ULONG celt, LPOLESTR *rgelt, @@ -102,6 +156,10 @@ public: *pceltFetched = 0; + CSLock lock(m_csStrings); + + DoUpdateIfNeeded(); + for ( const unsigned count = m_strings.size(); celt--; ++m_index ) { if ( m_index == count ) @@ -125,6 +183,8 @@ public: virtual HRESULT STDMETHODCALLTYPE Skip(ULONG celt) { + CSLock lock(m_csStrings); + m_index += celt; if ( m_index > m_strings.size() ) { @@ -137,6 +197,21 @@ public: virtual HRESULT STDMETHODCALLTYPE Reset() { + { + CSLock lock(m_csUpdate); + + if ( m_completer ) + { + // We will update the string from completer soon (or maybe are + // already in process of doing it) so don't do anything now, it + // is useless at best and could also result in a deadlock if + // m_csStrings is already locked by Next(). + return S_OK; + } + } + + CSLock lock(m_csStrings); + m_index = 0; return S_OK; @@ -147,6 +222,8 @@ public: if ( !ppEnum ) return E_POINTER; + CSLock lock(m_csStrings); + wxIEnumString *e = new wxIEnumString(*this); e->AddRef(); @@ -155,18 +232,86 @@ public: return S_OK; } + DECLARE_IUNKNOWN_METHODS; + private: // dtor doesn't have to be virtual as we're only ever deleted from our own // Release() and are not meant to be derived form anyhow, but making it // virtual silences gcc warnings; making it private makes it impossible to // (mistakenly) delete us directly instead of calling Release() - virtual ~wxIEnumString() { } + virtual ~wxIEnumString() + { + ::DeleteCriticalSection(&m_csStrings); + ::DeleteCriticalSection(&m_csUpdate); + } + + // Common part of all ctors. + void Init() + { + ::InitializeCriticalSection(&m_csUpdate); + ::InitializeCriticalSection(&m_csStrings); + + m_completer = NULL; + } + + void DoUpdateIfNeeded() + { + wxTextCompleter *completer; + wxString prefix; + { + CSLock lock(m_csUpdate); + completer = m_completer; + if ( !completer ) + return; + prefix = m_prefix; + } // Unlock m_csUpdate to allow the main thread to call our + // UpdateStringsFromCompleter() without blocking while we are + // generating completions. + + // Notice that m_csStrings is locked by our caller already so no need + // to enter it again. + m_index = 0; + m_strings.clear(); + completer->GetCompletions(prefix, m_strings); + + { + CSLock lock(m_csUpdate); + + if ( m_completer == completer && m_prefix == prefix ) + { + // There were no calls to UpdateStringsFromCompleter() while we + // generated the completions, so our completions are still + // pertinent. + m_completer = NULL; + return; + } + //else: Our completions are already out of date, regenerate them + // once again. + } + } + + + // Critical section protecting m_strings and m_index. + CRITICAL_SECTION m_csStrings; + + // The strings we iterate over and the current iteration index. wxArrayString m_strings; unsigned m_index; - friend class wxTextAutoCompleteData; + // This one protects just m_completer, it is different from m_csStrings + // because the main thread should be able to update our completer prefix + // without blocking (as this would freeze the UI) even while we're inside a + // possibly long process of updating m_strings. + CRITICAL_SECTION m_csUpdate; + + // If completer pointer is non-NULL, we must use it by calling its + // GetCompletions() method with the specified prefix to update the list of + // strings we iterate over when our Next() is called the next time. + wxTextCompleter *m_completer; + wxString m_prefix; + wxDECLARE_NO_ASSIGN_CLASS(wxIEnumString); }; @@ -282,7 +427,7 @@ public: void ChangeStrings(const wxArrayString& strings) { - m_enumStrings->m_strings = strings; + m_enumStrings->ChangeStrings(strings); DoRefresh(); } @@ -333,8 +478,7 @@ public: // probably should) use IAutoComplete::Enable(FALSE) for this too but // then we'd need to call Enable(TRUE) to turn it on back again later. - m_enumStrings->m_strings.clear(); - m_enumStrings->Reset(); + m_enumStrings->ChangeStrings(wxArrayString()); ChangeCustomCompleter(NULL); } @@ -372,11 +516,7 @@ private: const wxString prefix = m_entry->GetRange(0, from); - // For efficiency we access m_strings directly instead of creating - // another wxArrayString, normally this should save us an unnecessary - // memory allocation on the subsequent calls. - m_enumStrings->m_strings.clear(); - m_customCompleter->GetCompletions(prefix, m_enumStrings->m_strings); + m_enumStrings->UpdateStringsFromCompleter(m_customCompleter, prefix); DoRefresh(); } -- 2.45.2