Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Debugger] Record time it took between steps. #12217

Merged
merged 13 commits into from
Dec 31, 2018

Conversation

thaystg
Copy link
Contributor

@thaystg thaystg commented Dec 28, 2018

I implemented a new message to avoid breaking the protocol.
I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
There is a new unit test to check this feature.
Fixes #8460

…uest-arm64/8921/testReport/junit/(root)/DebuggerTests/StackTraceInNative/ for example, we have no idea why the underlying process has exited. To figure it out, we need to go to https://jenkins.mono-project.com/job/test-mono-pull-request-arm64/8921/parsed_console/log_content.html#WARNING1 and scroll back up to where the test was executed to find out the debuggee process crashed with an assertion.

This was fixed by redirecting the StandardOutput and StandardError of the debuggee process in the Mono.Debugger.Soft test suite.
I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
Copy link
Contributor

@Therzok Therzok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@thaystg
Copy link
Contributor Author

thaystg commented Dec 28, 2018

@monojenkins commit apidiff

@thaystg
Copy link
Contributor Author

thaystg commented Dec 28, 2018

@monojenkins build failed

I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
I implemented a new message to avoid breaking the protocol. I used the MonoStopwatch to count the time between the steps, it was already used in the debugger-agent.
Fixes mono#8460
@alexischr
Copy link
Contributor

@monojenkins commit apidiff

@@ -30,11 +30,20 @@ public class ThreadMirror : ObjectMirror
return frames;
}

public long ElapsedTime () {
long elapsedTime = GetElapsedTime ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this is split into also an internal method (ie. why not return vm.conn.Thread_GetElapsedTime (id))?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a good reason I was following the other implementations, but I really think that this is an unnecessary overhead. Should I change this and do a new PR?

monojenkins added a commit to mono/api-snapshot that referenced this pull request Dec 31, 2018
@alexischr alexischr merged commit 2a16c76 into mono:master Dec 31, 2018
@jbevain
Copy link
Member

jbevain commented Jan 18, 2019

@vargaz @thaystg @alexischr @marek-safar heads-up, this was merged without bumping the protocol version, introducing a potential binary protocol breakage.

The version of the server and client should be bumped.
The client should check that server has the supported version before calling the server to get the elapsed time.

@marek-safar
Copy link
Member

@thaystg could you address above ^

@thaystg
Copy link
Contributor Author

thaystg commented Jan 18, 2019

@thaystg could you address above ^

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants