Author Topic: codecompletion plugin lead codeblocks crash, do GUI operation in worker thread  (Read 36241 times)

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
ollydbg: The last patch does nothing interesting. Is it the only change that has to be made to C::B to fix the problem or other patches are required?
(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 ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
ollydbg: The last patch does nothing interesting. Is it the only change that has to be made to C::B to fix the problem or other patches are required?
Yes, the patch is not mature.
It is kind of proof of concept to move those operations in GUI main thread.  ;D
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: 13413
    • Travis build status
But if I understand you correctly you're moving all operations to the UI thread, because I don't see any data stored in the event's object.
(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 ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
But if I understand you correctly you're moving all operations to the UI thread, because I don't see any data stored in the event's object.
The function "void ClassBrowserBuilderThread::BuildTree()" is not called in "void* ClassBrowserBuilderThread::Entry()", but in "void ClassBrowser::OnBuileTree(cbBuildTreeEvent& event)".

The custom event(cbBuildTreeEvent) just carry a "ClassBrowserBuilderThread* thread" from the worker thread to main GUI.
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: 13413
    • Travis build status
OK, but how do you know how many elements you have to add to the tree in a single event handler? As far as I see it you still need locking.

If I'm doing this I'd rather:
1. in the parser thread generate some data and store it in the symbolbrowser event object
2. post an event with the gathered data
3. continue with parsing
4. when I receive the event I'll add the data to the tree

This way it is explicitly stated how owns the data exclusively and there is no need for locking.
(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 ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
OK, but how do you know how many elements you have to add to the tree in a single event handler? As far as I see it you still need locking.

If I'm doing this I'd rather:
1. in the parser thread generate some data and store it in the symbolbrowser event object
2. post an event with the gathered data
3. continue with parsing
4. when I receive the event I'll add the data to the tree

This way it is explicitly stated how owns the data exclusively and there is no need for locking.

OK, I understand you idea, but we are not talking with the same lockers.
You are stating the locker for the TokenTree, but my patch try to move the build wxTreeCtrl code from worker thread to GUI thread.

Note, TokenTree locker is still there, but mostly they don't cause issue, because the wxTreeCtrl is mainly constructed after the Parser finish the job, in that time, TokenTree is always free(not owned by other one).

Back to your proposed method, to avoid the TokenTree locker when building the tree, you need to construct some data structure (some kind of a copy of a sub Tokentree?) and send to the GUI thread, but if the data structure is too large?
« Last Edit: February 17, 2014, 01:53:58 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: 9694
Re:
« Reply #21 on: February 17, 2014, 06:13:19 am »
First of all the patch does not work but crashes cb at exit. Second if the data gets too large you can use a pointer. Third I don't think it's that easy.Yes, the method is called from outside but the task is still operated in the thread so it doesn't really change anything... Sorry.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re:
« Reply #22 on: February 17, 2014, 07:10:31 am »
First of all the patch does not work but crashes cb at exit.
Hi, Morten, thanks for the test. In-fact, If we need to build the Tree from GUI, we should totally remove the worker tree builder thread stuff.

Quote
Second if the data gets too large you can use a pointer.
Yes, just as the same way as we access to Tokentree to query suggestion list information.

Quote
Third I don't think it's that easy.Yes, the method is called from outside but the task is still operated in the thread so it doesn't really change anything... Sorry.
The tree build thread is large, also, I read some comments in the code like:
Code
// 9.) Expand item --> Bottleneck: Takes ~4 secs on C::B workspace
....
// 11.) Select the item saved before --> Bottleneck: Takes ~4 secs on C::B workspace
I won't let the GUI to hang for 8 seconds, so I won't step further in this direction for now.  :)
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 kipade

  • Multiple posting newcomer
  • *
  • Posts: 50
Hi All, thanks for all attention about this topic. I wanna say, the simple code was just a test for that, just to see if this can fixed the crash bug, a joke.
Yes, if need finally perfect fix for that issue, some more slight design for that might needed. Of course we can drive a tree control from wxTreeCtrl, it seems more familar with OO rule. But, there still have some other choices such as my moved BuidTree. I think, if all the operations could be separated with data, and GUI, one class only do which it should, all the things become easy. I think.
If it still no official support, I might continue to complete it by my hand, because I need it work under linux. Any idea is welcome.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
If it still no official support, I might continue to complete it by my hand, because I need it work under linux. Any idea is welcome.
Do you think it is definitely the codecompletion's browser tree which cause the crash issue?
I haven't seen that on our forum about such issue, can you show us the crash backtrace?
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: 13413
    • Travis build status
Are we sure what is taking all the time when building the tree from the main thread?
Has any one run a profiler to see what is slowing things down? Most of the times it is something quite unexpected, because people are really bad at guessing bottleneck!

If someone is willing to bring back all UI building code to the main thread and provide a patch, I'll run some profiling tools on it and I'll try to improve it.
(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: 9694
Re:
« Reply #26 on: February 17, 2014, 10:08:51 pm »
I'm afraid there is nothing to bring back as imho it has always been like that...
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
I think Loaden has moved it in a thread, but anyway, lets say to move the code to the UI thread in order to make it easier to profile.
(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 ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
I think Loaden has moved it in a thread, but anyway, ...
No, the wxTreeCtrl builder thread has a long history, it exist before Loaden became a C::B developer.
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 kipade

  • Multiple posting newcomer
  • *
  • Posts: 50
If it still no official support, I might continue to complete it by my hand, because I need it work under linux. Any idea is welcome.
Do you think it is definitely the codecompletion's browser tree which cause the crash issue?
I haven't seen that on our forum about such issue, can you show us the crash backtrace?

Im sure. I will upload a crash trace when I have some free time to have a try.