unnecessory locker in CClogger

Developer forums (C::B DEVELOPMENT STRICTLY!) > CodeCompletion redesign

(1/3) > >>

ollydbg:
Hi, when reading the code:

--- Code: ---void CCLogger::AddToken(const wxString& msg)
{
wxMutexLocker l(m_CCLoggerMutex);

if (!m_Parent || m_AddTokenId<1) return;

wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, m_AddTokenId);
evt.SetString(msg);
#if CC_PROCESS_LOG_EVENT_TO_PARENT
m_Parent->ProcessEvent(evt);
#else
wxPostEvent(m_Parent, evt);
#endif
}

void CCLogger::Log(const wxString& msg)
{
wxMutexLocker l(m_CCLoggerMutex);

if (!m_Parent || m_LogId<1) return;

wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, m_LogId);
evt.SetString(msg);
#if CC_PROCESS_LOG_EVENT_TO_PARENT
m_Parent->ProcessEvent(evt);
#else
wxPostEvent(m_Parent, evt);
#endif
}

void CCLogger::DebugLog(const wxString& msg)
{
wxMutexLocker l(m_CCLoggerMutex);

if (!m_Parent || m_DebugLogId<1) return;

wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, m_DebugLogId);
evt.SetString(msg);
#if CC_PROCESS_LOG_EVENT_TO_PARENT
m_Parent->ProcessEvent(evt);
#else
wxPostEvent(m_Parent, evt);
#endif
}

--- End code ---

Here, in the case of CC_PROCESS_LOG_EVENT_TO_PARENT is 0.

Refer to the link below, I think no wxMutexLocker should be used, because wxPostEvent is safe to send an event from a working thread to main GUI thread.

http://wiki.wxwidgets.org/Custom_Events#wxEvtHandler::ProcessEvent_vs._wxPostEvent_and_wxEvtHandler::AddPendingEvent

Any ideas/comments ?

dmoore:
One issue (i.e. crash) I've had with wxPostEvent (or AddPendingEvent) in the past is passing wxString between threads in a wxEvent object. Any time I passed a string, I had to make a copy of it with c_str() to avoid issues. I don't know if this is what the Mutex Locker in that code was trying to solve.

ollydbg:
Hi, dmoore, you are definitely right. I just debug a simple wx multiply thread app which link to the debug wx 2.8.12 library , so I can step into all the wx sources. I see that "wxPostEvent(m_Parent, evt);" will clone a new event:

--- Code: ---virtual wxEvent *Clone() const { return new wxCommandEvent(*this); }
--- End code ---

--- Code: --- wxCommandEvent(const wxCommandEvent& event)
   : wxEvent(event),
#if WXWIN_COMPATIBILITY_2_4
   m_commandString(this),
#endif
   m_cmdString(event.m_cmdString),
   m_commandInt(event.m_commandInt),
   m_extraLong(event.m_extraLong),
   m_clientData(event.m_clientData),
   m_clientObject(event.m_clientObject)
   { }

--- End code ---
But the bad thing is here:

--- Code: ---m_cmdString(event.m_cmdString)
--- End code ---
A wxString is construct from another wxString, so reference counting is used. Too bad, wxString's reference counting is not atomic variables (see my post several months ago wxString and the locker issue in our CC code)

Here, in the code:

--- Code: ---void CCLogger::AddToken(const wxString& msg)
{
   wxMutexLocker l(m_CCLoggerMutex);

   if (!m_Parent || m_AddTokenId<1) return;

   wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, m_AddTokenId);
   evt.SetString(msg);
#if CC_PROCESS_LOG_EVENT_TO_PARENT
   m_Parent->ProcessEvent(evt);
#else
   wxPostEvent(m_Parent, evt);
#endif
}

--- End code ---
I think the wxMutexLocker can NOT solve the thread-safe issue of wxString, as you said, here we must do a deep copy of wxString by ourselves, the only way use: evt.SetString(msg.c_str()); (See this post as in wx forum as a reference

EDIT:
wx since 2.9.0 has a nice event type: wxThreadEvent, which did a deep copy of every thing (in-fact, it did a deep copy of it's wxString)
see event.h in wx 2.9.0, there is also a "SetString(GetString().c_str());"

--- Code: ---// Thread event

class WXDLLIMPEXP_BASE wxThreadEvent : public wxEvent,
   public wxEventAnyPayloadMixin
{
public:
   wxThreadEvent(wxEventType eventType = wxEVT_THREAD, int id = wxID_ANY)
   : wxEvent(id, eventType)
   { }

   wxThreadEvent(const wxThreadEvent& event)
   : wxEvent(event),
   wxEventAnyPayloadMixin(event)
   {
   // make sure our string member (which uses COW, aka refcounting) is not
   // shared by other wxString instances:
   SetString(GetString().c_str());
   }

   virtual wxEvent *Clone() const
   {
   return new wxThreadEvent(*this);
   }

   // this is important to avoid that calling wxEventLoopBase::YieldFor thread events
   // gets processed when this is unwanted:
   virtual wxEventCategory GetEventCategory() const
   { return wxEVT_CATEGORY_THREAD; }

private:
   DECLARE_DYNAMIC_CLASS_NO_ASSIGN(wxThreadEvent)
};

--- End code ---

EDIT2:
In wx2.8.12, when build with GCC, the wxString is COW(copy on write, and reference counted), but the reference counter is not thread-safe(it does not use an atomic counter).
In wx2.9.x, when wxString build with GCC use GCC's native std::string, which is also COW, and it uses the atomic reference counter, so it is safe to use "evt.SetString(msg);".
But COW with atomic reference counter has performance issue(reported by Herb Sutter in his article Optimizations That Aren't (In a Multithreaded World)), so I see somewhere that GCC 5.0 or GCC 4.9 in the future may totally remove the COW feature of std::string. I once hear that Visual C++'s std::string is already NOT COW for years.

golgepapaz:

--- Quote from: ollydbg on June 20, 2013, 03:51:26 am ---
EDIT2:
In wx2.8.12, when build with GCC, the wxString is COW(copy on write, and reference counted), but the reference counter is not thread-safe(it does not use an atomic counter).
In wx2.9.x, when wxString build with GCC use GCC's native std::string, which is also COW, and it uses the atomic reference counter, so it is safe to use "evt.SetString(msg);".
But COW with atomic reference counter has performance issue(reported by Herb Sutter in his article Optimizations That Aren't (In a Multithreaded World)), so I see somewhere that GCC 5.0 or GCC 4.9 in the future may totally remove the COW feature of std::string. I once hear that Visual C++'s std::string is already NOT COW for years.

--- End quote ---

Heh I've burned because of that issue too. Mind you c++11 does not allow COW semantics for strings. But gcc still uses COW strings in 4.8.0 even with the -std=c++11 flag.Likely ABI change is the factor for delaying the transition here.

ollydbg:
I believe the using "c_str()" still have the thread safety issue, see my post in thread safety issue about wxCommandEvent in wx2.8.12

Navigation

[0] Message Index

[#] Next page

Go to full version