Attention massive speed up of the debugger

[oBFusCATed]

Hello,
here is a patch that improves the performance of the debugger massively.

The patch: http://smrt.is-a-geek.org/codeblocks/dbg_refactor/dbg_speedup.patch
It is against the debugger branch, but should apply in trunk (I've tested it and it worked against r5853)

How to test:
1. open some project
2. start debugging
3. stop on a breakpoint
4. open as many dbg windows as possible (call stack, threads, watches)
5. add many watches, complex variables are better (vectors will be great)
6. single step, through the code and see how the debugger toolbar is disable while the debugger is processing things

Execute the test before and after you've applied the patch.

What the patch does:
It adds a flag to the PipeProcess class, that controls if we will get notifications for every line of output or a single notification with the whole output.
The default value is set to notification for every line (the old behaviour).
With the patch only the debugger will use the new behaviour.

It seems that event passing is not so cheap in wx after all.

Have fun, feadback is quite welcome

p.s. I'm using the patch for some days now and I've not seen any problems only benefits

[oBFusCATed]

No one interested?  :shock: :? 

[thomas]

I tried to apply your patch, but it complains that debuggergdb.cpp and pipedprocess.h have some mismatching lines.

[oBFusCATed]

Can you post the output of the patch command?

I've tried to apply it on trunk and on the debugger branch and it works. (as I've written in my post)

[ollydbg]

No one interested?  :shock: :? 

I thought it was already applied in the trunk.

[thomas]

Can you post the output of the patch command?

I've tried to apply it on trunk and on the debugger branch and it works. (as I've written in my post)

Right-clicked with TortoiseSVN, and it said "fetching revision... (some number)" and then it said "the line (blah blah) does not match, maybe the file is out of date?"

[oBFusCATed]

Can you try with command line patch tool, because it spits more output?
I'm using (when I'm on windows) the one from UnixUtils and it seems to work.

[stahta01]

Applying to trunk; as of yesterday SVN ? Not on partition with full SVN Tools. It was updated yesterday sometime.

Tim S.

Code

patch --backup-if-mismatch --unified --strip=0 --ignore-whitespace --forward --input=..\Patches\temp\dbg_speedup-CRLF.patch
patching file src/plugins/compilergcc/compilergcc.cpp
patching file src/plugins/debuggergdb/debuggergdb.cpp
Hunk #1 succeeded at 1035 (offset 394 lines).
patching file src/sdk/pipedprocess.cpp
patching file src/sdk/toolsmanager.cpp
patching file src/include/pipedprocess.h
Hunk #2 succeeded at 35 with fuzz 1.

[thomas]

 
F:\codeblocks src>patch --backup-if-mismatch --unified --strip=0 --ignore-whitespace --forward --input=dbg_speedup.patch
patching file `src/plugins/compilergcc/compilergcc.cpp'
Assertion failed: hunk, file patch.c, line 321

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

[eranif]

F:\codeblocks src>patch --backup-if-mismatch --unified --strip=0 --ignore-whitespace --forward --input=dbg_speedup.patch
patching file `src/plugins/compilergcc/compilergcc.cpp'
Assertion failed: hunk, file patch.c, line 321

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

This usually means that the patch was created on Windows with CRLF ('\r\n', vim: ^M), just convert EOLs to LF ('\n')

Eran

[MortenMacFly]

No one interested?  :shock: :? 

Stay tuned... I have it applied since day 0, but was on holiday. If unsure after a (longer) while, simply PM me as a reminder.
@Thomas: Looks good to me so far, it's indeed a speedup.

[oBFusCATed]

I've figured it out
Just wanted to make some noise...

For the record it works perfectly on linux - arch 32bits, gentoo 64 bits ...

p.s. maybe other users (the compiler plugin in particular) of PipeProcess should be modified, so they can take advantage of the speedup

[Jenna]

I've figured it out
Just wanted to make some noise...

For the record it works perfectly on linux - arch 32bits, gentoo 64 bits ...

p.s. maybe other users (the compiler plugin in particular) of PipeProcess should be modified, so they can take advantage of the speedup

Your patch accidently slipped in r5876 (scrollingdialog) and it breaks debugging on my XP-machine, linux still works fine.
Creating the debuggers pipedprocess with eatEOL set to true also works, but with eatEOL set to false, debugger always hangs.
"Tested" with gdb 6.8.50 and 7.0.2.
I will revert the patch and try to find the problem (if possible).

EDIT:
Done (r5898),

sorry for any problems that might have occurred.

[MortenMacFly]

Your patch accidently slipped in r5876 (scrollingdialog) and it breaks debugging on my XP-machine, linux still works fine.

Huh? That works fine for me here (WinXP, too)...?! How do you "reproduce" the hang?

[Jenna]

Your patch accidently slipped in r5876 (scrollingdialog) and it breaks debugging on my XP-machine, linux still works fine.

Huh? That works fine for me here (WinXP, too)...?! How do you "reproduce" the hang?

Create a console-project, compile, set a breakpoint, start the debugger and the bp is never reached.
I also can not stop the debug-process, only kill it via taskmanager.

gdb uses 0% of my CPU and only very moderate ram (just a simple hello-world-program).

gcc 4.4.1 TDM-2 (sjlj),
gdb either shipped with TDM's MinGW (6.8 ) or 7.0.2.

As written before:

Creating the debuggers pipedprocess with eatEOL set to true also works, but with eatEOL set to false, debugger always hangs.

[oBFusCATed]

Maybe this code causes the problem:

Code

void GDB_driver::ParseOutput(const wxString& output)
{
    m_Cursor.changed = false;

    // Watch for initial debug info and grab the child PID
    // this is put here because we need this info even if
    // we don't get a prompt back.
    // It's "cheap" anyway because the line we 're after is
    // the very first line printed by gdb when running our
    // program. It then sets the child PID and never enters here
    // again because the "want_debug_events" condition below
    // is not satisfied anymore...
    if (platform::windows && want_debug_events)
    {
        wxRegEx* re = 0;
        if ((m_GDBVersionMajor > 6 || (m_GDBVersionMajor == 6 && m_GDBVersionMinor >= 7)) &&
            output.Contains(_T("CREATE_PROCESS_DEBUG_EVENT")))
        {
            re = &reChildPid2;
        }
        else if (m_GDBVersionMajor <= 6 && output.Contains(_T("do_initial_child_stuff")))
        {
            re = &reChildPid;
        }

        if (re)
.
.
.

Maybe we should do the splitting here

Code

void DebuggerGDB::ParseOutput(const wxString& output)
{
    if (!output.IsEmpty() && m_State.HasDriver())
    {
        m_State.GetDriver()->ParseOutput(output);
    }
}

Can you test?

[Jenna]

Maybe this code causes the problem:

[...]

Can you test?

I will test it, if I am at work.

[Jenna]

The problem we have, is that ParseOutput (in gdb_driver.cpp) relies on strings that belongs together and begin with special sequences, like gdb: or Warning:,  or do not contain anything but the response to one command (everything beginning with the GDB_PROMPT is removed inside the if(cmd)-clause).

But with your patch we can get output-strings that contain multiple lines of gdb's output.
This behaviour can break parsing (might depend on system load and whatever).

So the whole ParseOutput-function has to be overworked to work reliable with your patch.
The question is, whether the splitting and extra-parsing will eat up the time we save with the patch.

[oBFusCATed]

Do you have sample output from gdb? On linux I've no problems with it.

There is an easy fix you could try (pseudo code below):

Code

void DebuggerGDB::ParseOutput(const wxString& output)
{
    if (!output.IsEmpty() && m_State.HasDriver())
    {
        wxStringArray lines = split(output, '\n');

        for(int ii = 0; ii < lines.count(); ++ii)
             m_State.GetDriver()->ParseOutput(lines[ii]);
    }
}

This one should simulate the old behaviour...

[Jenna]

Your code does not work, here is a corrected version (EDIT: I overread that it is pseudo-code, sorry):

Code

void DebuggerGDB::ParseOutput(const wxString& output)
{
    if (!output.IsEmpty() && m_State.HasDriver())
    {
        wxArrayString lines = GetArrayFromString(output, _T('\n'));

        for(int i = 0; i < lines.GetCount(); ++i)
             m_State.GetDriver()->ParseOutput(lines[i]);
    }
}

I'm not sure if it can work reliably.
Can we reach OnGDBOutput, OnGDBError or the OnTimer-function while we are still parsing splitted output ?
If yes we would most likely mess up the order we send the data to the drivers ParseOutput.

Nevertheless, I attach a file containing the output of debugger's debug-log.
I just wrote the output-parameter from ParseOutput into the log.

[attachment deleted by admin]

[MortenMacFly]

There is an easy fix you could try (pseudo code below):

I am not sure if this would work. Can you be sure that the lines are complete? What if the first buffer contains 1.5 lines and the second buffer the second half of the second line (0.5 lines)? In that case the parser will not understand the second line of the first buffer and the first line of the second buffer. Thus one response is skipped.

To make it more clear... what I am talking about is a scenario like this:
Pseudo buffer1:

Code

gdb: whatever_message1 what_ever_param1
gdb: whatever_message2

Pseudo buffer2:

Code

 what_ever_param2

[oBFusCATed]

Hm, yes...
What about buffering if there is non terminated (\n) output?

[oBFusCATed]

OK, I've tried the buffering approach and it doesn't work  . It doesn't work because there messages that doesn't end with \n (the output from set prompt for example).

Morten: If somehow the message is split in two the ParseOutput code will break, with or without the patch! This is a problem of the used protocol. (you could prove me wrong though)

Jens: I've missed your message before: why the debug log has double '\n'?

[Jenna]

The additional crlf's should not be there. I'm quite sure I did not log additional line-endings, and the logs did not contain them.
I deleted the original files, so I can not compare them, to see where this strange thing happens.

If I insert a patch in the textbox while posting the same things happens sometimes( additional line-endings), so it might be an issue of the board software, even if I do not know how this should happen with attached files (but I know that MS Outlook does something similar with mail attachements).

Nevertheless you can ignore the additional newlines.


I don't think an output line can be split in the middle, and if it can, this might happen in your code and in the original code.

But as I wrote before:

Can we reach OnGDBOutput, OnGDBError or the OnTimer-function while we are still parsing splitted output ?
If yes we would most likely mess up the order we send the data to the drivers ParseOutput.

[oBFusCATed]

Can we reach OnGDBOutput, OnGDBError or the OnTimer-function while we are still parsing splitted output ?
If yes we would most likely mess up the order we send the data to the drivers ParseOutput.

What exactly are you talking about?
Entering OnGDBOutput while we are in ParseOutput or getting OnGDBError, OnGDBOutput, OnTimer in the middle of a result stream?

I think the first is not possible because the messages are in single thread.
And here is an example of the second situation:

Code

>input line1
>input line 2
>error line 1
>input line 3
>error line 2

[Jenna]

With your change we can get multiple lines of output, that have to be split into an array to work correctly in any cases as discussed before.
This will take some time (not really much, but that depeon the system's load and speed).

Then we loop through the array and send it to ParseOutput.

While this happens it might be that we reach another EVT_PIPEDPROCESS_STDOUT, EVT_PIPEDPROCESS_STDERR or EVT_TIMER, because the debugger process runs asynchronously, the timer should do this also of course.

So it might happen, that we are just parsing a large amount of output, while we get another event with content to parse, or am I missing something.

[oBFusCATed]

Jens can you provide some link documenting this kind of behaviour (I'm not an wx expert (not even wx advanced guy), most of the GUI code I've done is in MFC/win32 api),
I think we can't get a EVT_PIPEDPROCESS_STDOUT, EVT_PIPEDPROCESS_STDERR or EVT_TIMER,
While we are in a handler of some of them (ParseOutput is called only from handlers of these three messages).

Also: Can you test this patch: http://smrt.is-a-geek.org/codeblocks/dbg_speedup_test.patch

It does the splitting of the output,
logging to file ( change /home/obfuscated/cb.log to some path on your machine),
sends the non '\n' terminated output in the OnTimer handler.

If you have problems can you send me the result log file?
Also you could revert the changes in the DebuggerGDB::ParseOutput, rerun the debug session and send me the output file.

[Jenna]

No, I do not have any documentation about that (except for the wxwidgets docu).

It's just a question.

If it is sure, that we cannot get one of the events while we are processing one of them, there is no problem using this (splitting the messages in debuggergdb's ParseOutput and call the drivers ParseOutput for each line).

No problems here (until now) but I do not have much time for testing on windows at the moment.

[oBFusCATed]

No, I do not have any documentation about that (except for the wxwidgets docu).

It's just a question.

If it is sure, that we cannot get one of the events while we are processing one of them, there is no problem using this (splitting the messages in debuggergdb's ParseOutput and call the drivers ParseOutput for each line).

It can happen if someone does wxMesssageBox() or shows another modal dialog.
I've tested it with two timers. I don't know if there aren't any other cases where you can send/receive events from the queue inside a handler of another event? In win32 API you could do:

Code

	MSG	msg;
	while(PeekMessage(&msg, windowHandle, 0, 0, PM_REMOVE /*| PM_QS_PAINT*/) == TRUE)
	{
		TranslateMessage (&msg);
		DispatchMessage(&msg);
	}

to pop all messages from the queue.
Is there wx equivalent for this piece of code?

I'm thinking of a strategy how to fix this problem, but I don't have much time/energy at the moment

No problems here (until now) but I do not have much time for testing on windows at the moment.

Are you talking about the last patch?

[oBFusCATed]

What about this kind of solution:

Code

/// in the class declaration
    private:
        std::deque<wxString> m_lines;
        wxString m_timer1, m_timer2;

        bool m_in_handler;


/// in the cpp file
void wx_timerDialog::OnTimer(wxTimerEvent& event)
{
    m_timer1 = Common(m_timer1 + "t1_line1\nt1_line2\nt1_line3\n");
}

void wx_timerDialog::OnTimer2(wxTimerEvent& event)
{
    m_timer2 = Common(m_timer2 + "t2_line1\nt2_line2\n=t2_line3\n");
}

wxString wx_timerDialog::CommonFunc(wxString const &output)
{
    if(m_in_handler)
    {
        int start = 0;

        while(1)
        {
            wxString::size_type pos = output.find('\n', start);
            if(pos != wxString::npos)
            {
                m_lines.push_back(output.substr(start, pos - start));
                start = pos + 1;
            }
            else
                break;
        }

        if(start < output.length())
        {
            return output.substr(start, output.length() - start);
        }
    }
    else
    {
        m_in_handler = true;

        while(!m_lines.empty())
        {
            wxString const &s = m_lines.front();

            /// do some processing

            m_lines.pop_front();
        }

        m_in_handler = false;
    }
}

Any comments and suggestions are welcome.

[dmoore]

No, I do not have any documentation about that (except for the wxwidgets docu).

It's just a question.

If it is sure, that we cannot get one of the events while we are processing one of them, there is no problem using this (splitting the messages in debuggergdb's ParseOutput and call the drivers ParseOutput for each line).

It can happen if someone does wxMesssageBox() or shows another modal dialog.
I've tested it with two timers. I don't know if there aren't any other cases where you can send/receive events from the queue inside a handler of another event?

Based on some previous experience with this, I would think that so long as the handler does not invoke blocking operations that pump the message queue (e.g. modal dialogs) you should be ok. Think about the implications if this wasn't true: how could you do any reliable event handling if new events could interrupt and change the state of the app?

I don't think an output line can be split in the middle, and if it can, this might happen in your code and in the original code.

Couldn't a really really long string be split between buffers? I guess this can only happen if you happen to read the output quicker than it is being written by GDB (not impossible, right?). You could reduce the risk of this happening by continuing to read until IsInputAvailable() returns false (though even this may be error prone):

Code

    while(process->IsInputAvailable())
    {
        stdin_stream->Read(buffer,maxchars);
        //append buffer to wxstring or whatever
    }

[cbexaminr]

(oops, post was started in other thread, which hosts this quote:)

Haha, this is another problem an has nothing to do with filling the Disassembly window - http://forums.codeblocks.org/index.php/topic,11298.0.html

Unclear to me from this thread - is some form of this change/patch in current source (trunk? wxpropgrid_debugger branch?) at the moment or not?  (The actual patch doesn't seem to exist at the linked paths on smrt-is-a-geek anymore, so was unable to quickly check for myself.)

(Don't know if Jens reverted entire patch, or just some particular problematic part of it.  And then if that trunk revision was part of debugger branch, or not.)

If the problem it "creates" can already occur (as obfus indicated in one post there), I don't see why it wouldn't be (unless it made it much more likely to occur, as possibly happened for Jens)..

somewhat OT (related to other thread and operator+ vs .insert change in splitting debugger in two thread):
this is also, at least the suggestion using wxArrayStrings, a place where unnecessary allocations/processing would be done.  (Jens mentioned that possibility, as a possible no-win change.)  It appears (based on my reading of that thread) for (all) ParseOutput()s to remain happy, something somewhere has to do the newline parsing, it could be done around those parseoutput calls, without any add'l allocation(s) - the sticky point is handling the partial lines that may occur across buffer boundaries, although that should be possible as well.  The other sticky point would be output which will not be terminated by the '\n's, I believe you mentioned some 'set prompt' item as such a case. 

How does that 'set prompt' instance function now?

Is remote debugging (across _slow_ network) more susceptible in any way, to this possibility (split output received), or does something (gdb/server?) try to make sure an output "bundle" is sent/received atomically, at least for MI interface?

[oBFusCATed]

I understood almost nothing from your post...

Is remote debugging (across _slow_ network) more susceptible in any way, to this possibility (split output received), or does something (gdb/server?) try to make sure an output "bundle" is sent/received atomically, at least for MI interface?

The MI protocol is more robust and there aren't such problems (the protocols returns the whole result in one line, there are tags, etc...).

[cbexaminr]

I understood almost nothing from your post...

sorry... prob. not worth re-attempting at this time...

The MI protocol is more robust and there aren't such problems (the protocols returns the whole result in one line, there are tags, etc...).

Unclear -
Could the MI protocol implementation stand to benefit from your speed up attempts, if a "safe" version of it could be "found"?
Or has a similar level of performance improvement (to your attempted patch in trunk) already come from the use of the MI protocol (in the wxprogrid_debugger branch)?

(In other thread, you seemed to indicate the slowness is still there when you referenced this thread, so didn't know if the "such problems" included improved performance, or merely avoidance of line-ending issues...?)

[oBFusCATed]

The MI protocol has some advantages to the normal protocol:
1. it is asynchronous protocol - you can send multiple commands and process the output for every command when it is ready
2. the result from a command is one long line of text (most of the time), so the wx events sent from the PipedProcess is 1 event per command not N events per command as the normal protocol (this is the current performance problem)
3. the protocol is made to be useful to IDEs not humans, so it is very robust (no regex-s and such fragile operations)
4. you can query/update only the changed variables in the watches window (the current one updates all variables)....

So the GDB/MI plugin is pretty fast and I don't think it will benefit too much from the improvement related to the wx events.

The patch is not committed, because it is not reliable and it breaks the debugging. I think it could be made to work, but I've not tried lately...