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

Online 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

Online 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."

Online 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.

Online 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."

Online 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

Online 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