Author Topic: rev 7747 question: critical section enter and leave?  (Read 17853 times)

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5281
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
rev 7747 question: critical section enter and leave?
« on: February 01, 2012, 07:09:34 am »
Here, code: in classbrowser.cpp line 726
Code: [Select]

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

Why?
« Last Edit: February 01, 2012, 07:56:32 am by ollydbg »
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #1 on: February 01, 2012, 09:25:47 am »
Here, code: in classbrowser.cpp line 726
Code: [Select]

                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: [Select]

                THREAD_LOCKER_LEAVE(s_TokensTreeCritical);
                s_TokensTreeCritical.Leave();
Copy and paste error. :-(
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12913
    • Travis build status
Re: rev 7747 question: critical section enter and leave?
« Reply #2 on: February 01, 2012, 09:32:12 am »
Can I express my opinion that this macros are super ugly. Why don't you convert them to single lines?

Something like:
Code: [Select]
#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();
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #3 on: February 01, 2012, 09:38:11 am »
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.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: rev 7747 question: critical section enter and leave?
« Reply #4 on: February 01, 2012, 11:56:05 am »
I probably should not ask, but why lock/unlock manually at all when there's locker objects for that?
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5281
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: rev 7747 question: critical section enter and leave?
« Reply #5 on: February 01, 2012, 01:03:13 pm »
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.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12913
    • Travis build status
Re: rev 7747 question: critical section enter and leave?
« Reply #6 on: February 01, 2012, 01:08:49 pm »
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.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #7 on: February 01, 2012, 01:17:59 pm »
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.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #8 on: February 01, 2012, 01:19:35 pm »
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.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: rev 7747 question: critical section enter and leave?
« Reply #9 on: February 01, 2012, 02:07:24 pm »
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?
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5281
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: rev 7747 question: critical section enter and leave?
« Reply #10 on: February 01, 2012, 02:22:03 pm »
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: [Select]
#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?

Quote
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.
« Last Edit: February 01, 2012, 02:24:57 pm by ollydbg »
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #11 on: February 01, 2012, 02:38:05 pm »
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).
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #12 on: February 01, 2012, 02:40:14 pm »
How can you reference the caller in the locker's destructor?
A good reason for the macros, btw. ;-)
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5281
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: rev 7747 question: critical section enter and leave?
« Reply #13 on: February 01, 2012, 02:56:56 pm »
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... :)

If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #14 on: February 01, 2012, 03:05:51 pm »
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...)
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5281
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: rev 7747 question: critical section enter and leave?
« Reply #15 on: February 01, 2012, 03:22:38 pm »
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)
Quote
Just the one in the tokens, or are there more?
There are many wxStrings in TokensTree class.


Quote
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.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: rev 7747 question: critical section enter and leave?
« Reply #16 on: February 01, 2012, 05:14:29 pm »
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.

Quote
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: [Select]
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.

Quote
other reason is [...] wxString is not thread safe
No reason, really. Strings are only ever modified from one thread concurrently.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: rev 7747 question: critical section enter and leave?
« Reply #17 on: February 01, 2012, 05:20:34 pm »
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.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9606
Re: rev 7747 question: critical section enter and leave?
« Reply #18 on: February 01, 2012, 05:45:12 pm »
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.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5281
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: rev 7747 question: critical section enter and leave?
« Reply #19 on: February 02, 2012, 01:58:46 am »
Quote
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
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.