CodeCompletion Close project anomally

[Pecan]

svn build  rev 10554 (2015-11-01 09:18:34)   gcc 4.9.2 Windows/unicode - 32 bit

I've been tracing through CC to see why it can take nearly 30 seconds to close CodeBlocks.cbp if it's closed before parsing completes.

I've found that the TerminateAllThreads routine called from OnProjectClose()
sets a flag (m_abort) true at one address while IS_ALIVE in parserthread (aka TestDestroy()) reads the (m_abort) flag (which is always false) from a completely different address. So CC never sees that it should abort parsing.

Could someone point me in the right direction as to where I might look to resolve the two different flags anomaly?

Thanks
Lane (aka Pecan)



 

[ollydbg]

Interesting, I just debugged a little, I first set a breakpoint in the line:

Code

inline void cbThreadedTask::Abort()
{
  m_abort = true;
}

When I close the project, I do see this value is changed to true, and I see its address by

Code

[debug]> p &m_abort
[debug]$2 = (bool *) 0xd9e3efc

And I see that

Code

[debug]> p m_threads
[debug]$3 = std::vector of length 1, capacity 1 = {0xd555fb8}

This means only one working task is running, and we have set the flag to true.

After that I set a breakpoint in the line:

Code

inline bool cbThreadedTask::TestDestroy() const
{
  return m_abort;
}

When I run the debugee, I see it hits the bp, and I see the m_abort, it is still false, and its address is

Code

[debug]> p &m_abort
[debug]$5 = (bool *) 0xda25fec

This means they are quite different address. I will look into it.

Normally, when a task is running in the thread pool(note that our thread pool only runs one thread), when you try to stop the parsing, the running task should be stopped, and the queued task should be deleted.

[ollydbg]

OK, I know the reason, because we create some ParserThread instance inside the ParserThread::DoParse() function. That is how we handle #include "xxx.h". Each file(either a header file or a cpp file) is associated with a ParserThread object.(The TokenTree need to assignment every file an index number, so we can handle Tokens belong to different files)
I mean, if we meet such #include statement, a new ParserThread object is created, and we recursively parse into the xxx.h file by running the embedding ParserThread object.

And when we set the stop flag, we only set the m_abort member variable of the top level ParserThread instance, and we test the stop flag in all the embedding ParserThread object. That's the point!

How can we solve this issue?
Do not create an embedding ParserThread objects? This looks not quite good, since ParserThread contains some file index number, and buffer, so we did need new ParserThread objects. Share/inherit the m_abort value with all the embedding ParserThreads? (How to do that? a new class Type named EmbedingParserThread?)

[Pecan]

Thanks Ollydbg,
 
After doing more tracing, I understand your explanation.

I also see that just after setting m_abort, the parserThreads are immediately deleted by
cbThreadPool::AbortAllTasks()

Code

  std::for_each(m_tasksQueue.begin(), m_tasksQueue.end(), std::mem_fun_ref(&cbThreadedTaskElement::Delete));

so it doesn't even matter if each ParserThread is posted to abort or not.
Although they are in fact posted by cbThreadPool at

Code

void cbThreadPool::cbWorkerThread::AbortTask()
{
  wxMutexLocker lock(m_taskMutex);

  if (m_pTask)
    m_pTask->Abort();
}

But traping on the ParserThreads call to cbThreadedTask::TestDestroy() always shows that m_abort is always false, never true. (except for the main Parser thread you mentioned above.

Code

inline bool cbThreadedTask::TestDestroy() const
{
  return m_abort;
}

So I'm guessing, then, that the 20 seconds (on my laptop) to close CodeBlocks.cbp (windows 7) is caused by the delete of each ParserThread. About 432 of them (give or take a few when I hit close project).

I'll do some more tracing to confirm that.

My objective is to find a way to post abort or delete those threads in a faster manner.

This may be folly on my part, but I have the nasty habit of opening, viewing, closing CodeBlocks.cbp before parsing finishes, then having to wait nearly 30 seconds for it to close. Half the time, CB shows the "... is not responding" message from Windows 7.

Thanks for the explanation.

   

[ollydbg]

I also see that just after setting m_abort, the parserThreads are immediately deleted by
cbThreadPool::AbortAllTasks()

Code

  std::for_each(m_tasksQueue.begin(), m_tasksQueue.end(), std::mem_fun_ref(&cbThreadedTaskElement::Delete));

so it doesn't even matter if each ParserThread is posted to abort or not.
Although they are in fact posted by cbThreadPool at

Code

void cbThreadPool::cbWorkerThread::AbortTask()
{
  wxMutexLocker lock(m_taskMutex);

  if (m_pTask)
    m_pTask->Abort();
}

I'm not sure, but looked at your description, I think you are not fully understand the logic here, let me explain.

The code:

Code

void cbThreadPool::AbortAllTasks()
{
  wxMutexLocker lock(m_Mutex);

  std::for_each(m_threads.begin(), m_threads.end(), std::mem_fun(&cbWorkerThread::AbortTask));
  std::for_each(m_tasksQueue.begin(), m_tasksQueue.end(), std::mem_fun_ref(&cbThreadedTaskElement::Delete));
  m_tasksQueue.clear();
}

1, Tasks are running in the threads one by one. m_threads are all the running threads. m_tasksQueue are the queued tasks, and they are not running.
2, when you call cbWorkerThread::AbortTask(), it just mark the running task's flag to true:

Code

inline void cbThreadedTask::Abort()
{
  m_abort = true;
}

So that the running task has a chance to exit.

3, now, if you have a project, which contains 3 cpp files.

Code

a.cpp -> x.h -> y.h -> z.h ->...
b.cpp -> x.h -> y.h -> z.h ->...
c.cpp -> x.h -> y.h -> z.h ->...

"->" stands for "#include" directive.
When the parser start to work, it create three tasks(that's one ParserThread object per a cpp file) and put them in the thread pool. Our thread pool currently only allow one running thread, because we can avoid multiply threads to access the symbol table(TokenTree).

Now, we are running the ParserThread associated with a.cpp, and if will create a new ParserThread x.h when it see a #include "x.h" directive. But please not that at this time, the ParserThread associated with x.h is not put into the thread pool, it just runs inside the top level(the a.cpp's) ParserThread's DoParse() function, it may recursively create another Parserthread object if it see a "#include y.h".

When you try to abort the parsing, you set the flag m_abort on the a.cpp's ParserThread, this is the running task, but the bad thing is, you are actually running the embedding y.h's ParserThread, so this task won't finished immediately, until all the header files are parsed, and the control flow goes back to the top level a.cpp's ParserThread, so it will take a lot of time!

The two ParserThread objects associated with b.cpp and c.cpp are the tasked put in the m_tasksQueue, so they will be deleted without any delay.

But traping on the ParserThreads call to cbThreadedTask::TestDestroy() always shows that m_abort is always false, never true. (except for the main Parser thread you mentioned above.

Code

inline bool cbThreadedTask::TestDestroy() const
{
  return m_abort;
}

So I'm guessing, then, that the 20 seconds (on my laptop) to close CodeBlocks.cbp (windows 7) is caused by the delete of each ParserThread. About 432 of them (give or take a few when I hit close project).

I'll do some more tracing to confirm that.

My objective is to find a way to post abort or delete those threads in a faster manner.

This may be folly on my part, but I have the nasty habit of opening, viewing, closing CodeBlocks.cbp before parsing finishes, then having to wait nearly 30 seconds for it to close. Half the time, CB shows the "... is not responding" message from Windows 7.

Thanks for the explanation.

For my example I exaplaned above, you 20 second are just waiting for the ParserThread associated with a.cpp to finish. Currently we need to find a way to exit the embedding(inner) Parserthread quickly.

[Pecan]

Thanks again !!

That really clears up a mystery for me.

I'll look for some clever way to get the recursive .h threads to check for something like the main thread calls TestDestroy().

Here we go.... more hours of stepping through CodeCompletion.

[Pecan]

ollydbg,

I'd like to propose a patch to parser.h/cpp that records the internal threads (just like cbthreadpool) and posts Abort() to the internal threads when ~parser is invoked.

Initial testing with the patch shows that closing a large project (like CodeBlocks.cbp) terminates very fast (without the long delay previously experienced).

If you agree/modify the patch, we can later clean it up (removing //(pecan 2015/11/13) tags which I use to find mods fast) and apply.

I also include one line patches to the logger code to avoid crashes I've experienced a few times when closing CB while CC was busy.

svn build  rev 10554 (2015-11-01 09:18:34)   gcc 4.9.2 Windows/unicode - 32 bit

Code

Index: src/plugins/codecompletion/parser/cclogger.cpp
===================================================================
--- src/plugins/codecompletion/parser/cclogger.cpp	(revision 10554)
+++ src/plugins/codecompletion/parser/cclogger.cpp	(working copy)
@@ -87,6 +87,10 @@
 
 void CCLogger::Log(const wxString& msg)
 {
+    //(pecan 2015/11/12) could crash here; should check if shutting down
+    if (Manager::IsAppShuttingDown())
+        return;
+
     if (!m_Parent || m_LogId<1) return;
 
     CodeBlocksThreadEvent evt(wxEVT_COMMAND_MENU_SELECTED, m_LogId);
@@ -100,6 +104,10 @@
 
 void CCLogger::DebugLog(const wxString& msg)
 {
+    //(pecan 2015/11/12) could crash here; should check if shutting down
+    if (Manager::IsAppShuttingDown())
+        return;
+
     if (!m_Parent || m_DebugLogId<1) return;
 
     CodeBlocksThreadEvent evt(wxEVT_COMMAND_MENU_SELECTED, m_DebugLogId);
Index: src/plugins/codecompletion/parser/parser.cpp
===================================================================
--- src/plugins/codecompletion/parser/parser.cpp	(revision 10554)
+++ src/plugins/codecompletion/parser/parser.cpp	(working copy)
@@ -338,7 +338,9 @@
 
             TRACE(_T("Parser::Parse(): Parsing included header, %s"), filename.wx_str());
             // run the parse recursively
+            AddParserThread(thread);
             result = thread->Parse();
+            RemoveParserThread(thread);
             delete thread;
             return true;
         }
@@ -538,6 +540,7 @@
     // NOTE: This should not be locked with s_ParserMutex, otherwise we'll be stuck in an
     // infinite loop below since the worker thread also enters s_ParserMutex.
     // In fact cbThreadPool maintains it's own mutex, so m_Pool is probably threadsafe.
+    AbortParserThreads();
     m_Pool.AbortAllTasks();
     while (!m_Pool.Done())
         wxMilliSleep(1);
@@ -919,3 +922,38 @@
     // Page "Documentation:
     // m_Options.storeDocumentation will be written by DocumentationPopup
 }
+
+void Parser::AddParserThread (cbThreadedTask *task) //(pecan 2015/11/13)+
+{
+    if (!task)
+        return;
+
+    m_tasksQueue.push_back (task);
+
+}
+void Parser::RemoveParserThread (cbThreadedTask* task) //(pecan 2015/11/13)+
+{
+    if (!task)
+        return;
+
+    for (TasksQueue::iterator it = m_tasksQueue.begin(); it != m_tasksQueue.end(); ++it)
+    {
+        if ((*it) == task)
+        {
+            m_tasksQueue.erase(it);
+            break;
+        }
+
+    }
+}
+void Parser::AbortParserThreads() //(pecan 2015/11/13)+
+{
+    if (not m_tasksQueue.size())
+        return;
+
+    for (TasksQueue::iterator it = m_tasksQueue.begin(); it != m_tasksQueue.end(); ++it)
+    {
+            (*it)->Abort();
+
+    }
+}
Index: src/plugins/codecompletion/parser/parser.h
===================================================================
--- src/plugins/codecompletion/parser/parser.h	(revision 10554)
+++ src/plugins/codecompletion/parser/parser.h	(working copy)
@@ -355,6 +355,13 @@
 
     /** if true, all the files of the current project will be labeled as "local" */
     bool                      m_NeedMarkFileAsLocal;
+
+    typedef std::list<cbThreadedTask*> TasksQueue; //(pecan 2015/11/13)+
+    TasksQueue m_tasksQueue;    //(pecan 2015/11/13)+
+    void AddParserThread(cbThreadedTask* task); //(pecan 2015/11/13)+
+    void RemoveParserThread(cbThreadedTask* task); //(pecan 2015/11/13)+
+    void AbortParserThreads(); //(pecan 2015/11/13)+
+
 };
 
 #endif // PARSER_H 

[oBFusCATed]

Hm, Am I correct in thinking that the need for Manager::IsAppShuttingDown() is a sign that there is a bug somewhere else?

[Pecan]

Hm, Am I correct in thinking that the need for Manager::IsAppShuttingDown() is a sign that there is a bug somewhere else?

I'm not sure that's the case.

Way back in CB history, we went through a period where CB often crashed when being shutdown because something about the logs had already been destroyed when plugins attempted to use them during shutdown.

It was some kind of race condition with plugin termination. But now I'm getting too old to remember what it was.

Martin or Lievin might remember.

[MortenMacFly]

Yes, I recall something like the logger gets re- intantiated after already being destroyed... Wasn't it like that?

Btw, Pecan - nice work, although I didn't experience this delay on daily work I can reproduce and also confirm that the patch fixes it.

[ollydbg]

ollydbg,

I'd like to propose a patch to parser.h/cpp that records the internal threads (just like cbthreadpool) and posts Abort() to the internal threads when ~parser is invoked.

Initial testing with the patch shows that closing a large project (like CodeBlocks.cbp) terminates very fast (without the long delay previously experienced).

If you agree/modify the patch, we can later clean it up (removing //(pecan 2015/11/13) tags which I use to find mods fast) and apply.

I also include one line patches to the logger code to avoid crashes I've experienced a few times when closing CB while CC was busy.

svn build  rev 10554 (2015-11-01 09:18:34)   gcc 4.9.2 Windows/unicode - 32 bit

Great work, I see you use quite a smart idea to solve this issue. The patch is OK to commit from my side.
Here is what I suggest:
1, I think handling #include is just like a "push" and "pop" file stack. When see a #include directive, a new ParserThread is created, it is pushed to the stack, and leave a header file will pop the ParserThread from the stack, so some code can be simplified(no need to loop on the full list elements). In the future the stack can used to generate all the file dependency.
2, I would suggest adding some doxygen style document before the function prototype.

[Pecan]

Patch (with minor changes) applied SVN 10571.

Thanks everyone, especially ollydbg for the time spent guiding me through CodeCompletion.

lane