rev 7747 question: critical section enter and leave?

[ollydbg]

Here, code: in classbrowser.cpp line 726

Code

                THREAD_LOCKER_ENTER(s_TokensTreeCritical);
                s_TokensTreeCritical.Enter();
                THREAD_LOCKER_ENTERED(s_TokensTreeCritical);
                THREAD_LOCKER_LEAVE(s_TokensTreeCritical);
                s_TokensTreeCritical.Leave();

Why?

[MortenMacFly]

Here, code: in classbrowser.cpp line 726

Code

                THREAD_LOCKER_ENTER(s_TokensTreeCritical);
                s_TokensTreeCritical.Enter();
                THREAD_LOCKER_ENTERED(s_TokensTreeCritical);
                THREAD_LOCKER_LEAVE(s_TokensTreeCritical);
                s_TokensTreeCritical.Leave();

Why?

You found a bug, it should be:

Code

                THREAD_LOCKER_LEAVE(s_TokensTreeCritical);
                s_TokensTreeCritical.Leave();

Copy and paste error. :-(

[oBFusCATed]

Can I express my opinion that this macros are super ugly. Why don't you convert them to single lines?

Something like:

Code

#define CB_LOCKER_ENTER(lock) \
     THREAD_LOCKER_ENTER(lock); \
     lock.Enter(); \
     THREAD_LOCKER_ENTERED(lock);

#define CB_LOCKER_LEAVE(lock) \
      THREAD_LOCKER_LEAVE(lock); \
      lock.Leave(); 

[MortenMacFly]

Can I express my opinion that this macros are super ugly. Why don't you convert them to single lines?

That would be the next logical step, but it got too late yesterday and actually I wasn't aware that they are REALLY all over the place. :-( And BTW: This affects not only critical sections, but also mutexes and semaphores.

[thomas]

I probably should not ask, but why lock/unlock manually at all when there's locker objects for that?

[ollydbg]

I probably should not ask, but why lock/unlock manually at all when there's locker objects for that?

I guess this way, we can log the information in both entering and leave the critical section stage. The locker scheme can only log the entering info.

[oBFusCATed]

The locker scheme can only log the entering info.

Wrong, no one stops you writing another wrapper for the Locker class
Or you can just write you own version of it: call enter in the constructor and leave in the destructor.

[MortenMacFly]

I probably should not ask, but why lock/unlock manually at all when there's locker objects for that?

Because I want to track the error in return and want to have a specific protocol.

[MortenMacFly]

Or you can just write you own version of it: call enter in the constructor and leave in the destructor.

Yes, but I also would like to explicitly SEE in the code where session are entered and where they are left for clarity. Actually I am doing this to trace a deadlock. In the end it will be very simple.

[thomas]

Well it's too late for that after you've spent many hours... but I would just have made my own locker class identical to the original wxMutexLocker with logging added in constructor and destructor, and added a #define wxMutexLocker myLoggingMutexLocker when desired. When exactly the section is entered and left is clear: enter when the locker object is created, leave when the scope ends (next closing curly brace).

Good luck on finding the deadlock though, this is some nasty work.  

Why are there so many locks/unlocks anyway? I counted 85 in the 3 CC files, that seems like an awful lot. I've never really touched code completion because it always scared me off... but naively thought, wouldn't one normally want to parse one complete file at a time (in N threads), and only lock/unlock the shared data structure to flush in all newly parsed tokens once after finishing each file? Something like that?

[ollydbg]

Well it's too late for that after you've spent many hours... but I would just have made my own locker class identical to the original wxMutexLocker with logging added in constructor and destructor, and added a #define wxMutexLocker myLoggingMutexLocker when desired. When exactly the section is entered and left is clear: enter when the locker object is created, leave when the scope ends (next closing curly brace).

The macro can have the current function and line information

Code

#define THREAD_LOCKER_ENTER(NAME)                                            \
        CCLogger::Get()->DebugLog(F(_T("%s.Enter() : %s(), %s, %d"),             \
                                    wxString(#NAME, wxConvUTF8).wx_str(),        \
                                    wxString(__FUNCTION__, wxConvUTF8).wx_str(), \
                                    wxString(__FILE__, wxConvUTF8).wx_str(),     \
                                    __LINE__))
    #define THREAD_LOCKER_ENTERED(NAME)                                          \
        CCLogger::Get()->DebugLog(F(_T("%s.Entered() : %s(), %s, %d"),           \
                                    wxString(#NAME, wxConvUTF8).wx_str(),        \
                                    wxString(__FUNCTION__, wxConvUTF8).wx_str(), \
                                    wxString(__FILE__, wxConvUTF8).wx_str(),     \
                                    __LINE__))
    #define THREAD_LOCKER_LEAVE(NAME)                                            \
        CCLogger::Get()->DebugLog(F(_T("%s.Leave() : %s(), %s, %d"),             \
                                    wxString(#NAME, wxConvUTF8).wx_str(),        \
                                    wxString(__FUNCTION__, wxConvUTF8).wx_str(), \
                                    wxString(__FILE__, wxConvUTF8).wx_str(),     \
                                    __LINE__))

How can you reference the caller in the locker's destructor?

Why are there so many locks/unlocks anyway? I counted 85 in the 3 CC files, that seems like an awful lot. I've never really touched code completion because it always scared me off... but naively thought, wouldn't one normally want to parse one complete file at a time (in N threads), and only lock/unlock the shared data structure to flush in all newly parsed tokens once after finishing each file? Something like that?

There are too many lockers(I hate them , because we want to access to the TokensTree from either GUI or working thread. So, we need at least a locker of the TokensTree.
The other reason is that: wxString is not thread safe(wxString is reference counted, but the counter is not thread safe, std::string is reference counted, but its counter is thread safe)

Edit:
wxWidgets 2.9.x use std::string/std::wstring internally for wxString, thus it is much safer than 2.8.x version.

[MortenMacFly]

Why are there so many locks/unlocks anyway?

Most of them are if the tokens tree is accessed either from the parser to fill it or from the UI to obtain information.
The second most are critical sections whenever a parser changes (options or actually parses a file).

[MortenMacFly]

How can you reference the caller in the locker's destructor?

A good reason for the macros, btw. ;-)

[ollydbg]

The worse thing I can imagine is that:
1, we have TokensTree: TreeA and TreeB
2, we have two parserthread ThreadA and ThreadB
Now, ThreadA only accesses TreeA, ThreadB only accesses TreeB.

I once asked Loaden: why in this case, we still have a s_TokensTreeCritical to protect the access to different trees. Also Why we can't concurrently run two Parserthreads like the above case?

He explained to me that the reason is "wxString is not thread safe", this means some wxString's contents may be shared in both TreeA and TreeB, as the wxString(wx2.8.x) can not protect their reference counter in multiply threads accessing.

That's the reason that even we are in "One parser object for One cb project" mode (In this case, we have many Parser objects, and each Parser instance have its own TokensTree), we still need a static s_TokensTreeCritical.

So bad...

[MortenMacFly]

He explained to me that the reason is "wxString is not thread safe", this means some wxString's contents may be shared in both TreeA and TreeB, as the wxString(wx2.8.x) can not protect their reference counter in multiply threads accessing.

What strings are these? Just the one in the tokens, or are there more? Why don't we simply replace them with "something" thread-safe? (One reason I could think of not so easy with something else than wxString is encoding conversion btw...)

[ollydbg]

He explained to me that the reason is "wxString is not thread safe", this means some wxString's contents may be shared in both TreeA and TreeB, as the wxString(wx2.8.x) can not protect their reference counter in multiply threads accessing.

What strings are these?

You mean what strings whose reference counting is thread safe? std::sting and std::wstring under gcc/mingw are this kind of strings. (the std::string under Visual c++ 05?08? or later even do not have reference count feature, so if you copy them, you have a deep copy, that's not a big deal in modern CPUs)

Just the one in the tokens, or are there more?

There are many wxStrings in TokensTree class.

Why don't we simply replace them with "something" thread-safe? (One reason I could think of not so easy with something else than wxString is encoding conversion btw...)

I don't know the reason, it looks like we should DO this. Though the next generation of wxWidgets (2.9.x) have internally use std::string/wstring, but it have another issue. This "another issue" is:

Under Linux, the wxString is internally a std::string, and all the strings are recorded in UTF8 format.
Under Windows, the wxString is internally a std::wstring, and all the strings are recored in UTF16 format(the name under Windows we call it UNICODE format or wide char format)

You see, under Linux, we may have potential problems to access a wxChar(not sure whether under wx2.9.x we can still use wxChar) through an index value, because it use a variable length encoding.

[thomas]

How can you reference the caller in the locker's destructor?

A good reason for the macros, btw. ;-)

Not really  
First, being agnostic of a better solution, you can get this very effect by wrapping the locker's constructor in a macro and forwarding the constants. You'll still have a macro, but you'll retain exception safety and avoid generating more deadlocks.

Second, it's not necessary at all, since gcc allows you to query a function's return address without any hassle. addr2line translates the address back. I'm using this technique in one of my projects to track where an exception that made it to the outermost handler came from, this works like a charm.

There are too many lockers [...] access to the TokensTree from either GUI or working thread

Ok, this is a reason for one locker, but not for many. Normally, the high-level flow of a worker would be something like this:

Code

while(get_next_file())
{
    while(token = parse())
        my_list.push_back(token);

    do{
        locker(lock);
        for(auto t : my_list) ParseTree.add(t);
    }while(0);
}

Of course you wouldn't just store tokens in a list (makes the example simpler, though). But nothing hinders you to build your own parse sub-tree and have a function that merges a sub-tree into the main tree, that's just an implementation detail.

other reason is [...] wxString is not thread safe

No reason, really. Strings are only ever modified from one thread concurrently.

[thomas]

To explain what I mean with "avoid generating more deadlocks", the scary thing about these macros is that they may create deadlocks which would not otherwise be there.

If an exception is thrown between lock and unlock, you have a deadlock (in addition to any you might have had before). Even if the exception is gracefully handled (which admittedly is often not the case anyway).

Now with a locker, you automatically and unconditionally clean up in presence of an exception, which includes unlocking.

[MortenMacFly]

Now with a locker, you automatically and unconditionally clean up in presence of an exception, which includes unlocking.

Look: What I am trying to do it TO GET RID of so many lockers. But to do so, I must know how the message flow goes. So all this macro stuff is just there for debugging.
In the end, you can easily adjust the macro later at any time to use a locker object - simply by changing the ENTER/LOCK macro to create an object and changing the LEAVE/UNLOCK macro to turn into nothing. Easy, isn't it?

Look at r7763 (cclogger.h) - I've done it similar to detect dead locks in none-debug mode.

Again: The final goal is t remove this stuff, not to keep it.

[ollydbg]

other reason is [...] wxString is not thread safe

No reason, really. Strings are only ever modified from one thread concurrently.

I'm not saying that two thread are accessing one string instance, I'm saying that two thread are accessing two instances. Like stringA and stringB, but maybe, stringB is copied from stringA, they internally share contents. E.g. A parserthread collect a Token(stringA), and store it in TokensTree(stringB in TokensTree).

As stringA and stringB are reference counted, if reference counter is thread safe, then the above situation is OK. But wxString's reference counter is not thread safe.

This is the article talking about this kind of issue and performanceOptimizations That Aren't (In a Multithreaded World), currently gcc use the atomic variables and atomic functions to protect the reference counter in std::string/wstings. See:C/C++ reference counting with atomic variables and gcc - Alex on Linux