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

Diagnostic improvements #53

Closed
AndreyAkinshin opened this Issue Nov 28, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@AndreyAkinshin
Member

AndreyAkinshin commented Nov 28, 2015

@mattwarren, I want the following list of features:

  • 1. I want be able to add a diagnostic class with the AddDiagnoster method in manual mode (automatic loading should work in audo mode).
  • 2. I want a special diagnostic BenchmarkIterationMode for diagnostic. It should be run before the pre-warmup stage.
  • 3. Now I have troubles with diagnostic of x86 benchmark from the x64 host application. Can we fix it?
  • 4. It would be cool to split the current diagnostic class to source diagnoster (IL and ASM) and runtime diagnoster (GC, Segments, and so on).
  • 5. We should cover the diagnostics by unit tests.

What do you think?

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Nov 29, 2015

Member

I have tried to implement tasks 1 and 4. Please, review (2808348).
Now you can add specific diagnostic with a command like arguments like -d=source,runtime (d means diagnostic, target diagosers will be matched by name).

Member

AndreyAkinshin commented Nov 29, 2015

I have tried to implement tasks 1 and 4. Please, review (2808348).
Now you can add specific diagnostic with a command like arguments like -d=source,runtime (d means diagnostic, target diagosers will be matched by name).

@mattwarren

This comment has been minimized.

Show comment
Hide comment
@mattwarren

mattwarren Nov 30, 2015

Contributor

I have tried to implement tasks 1 and 4. Please, review (2808348).

Thanks for doing that, at first glance it looks good, but I'll have a more of a look later on today.

2 - I want a special diagnostic BenchmarkIterationMode for diagnostic. It should be run before the pre-warmup stage.

Just curious, what's the reason for this? Is it so the diagnostic doesn't interfere with calculating the invokeCount

3 - Now I have troubles with diagnostic of x86 benchmark from the x64 host application. Can we fix it?

Yeah that's a problem with the CLRMD library, it doesn't want to analyse a 32-bit process from a 64-bit host app. I'll take a look, I think I can fix it by having a small shim 32-bit process that can then launch the 32-bit benchmark.

Contributor

mattwarren commented Nov 30, 2015

I have tried to implement tasks 1 and 4. Please, review (2808348).

Thanks for doing that, at first glance it looks good, but I'll have a more of a look later on today.

2 - I want a special diagnostic BenchmarkIterationMode for diagnostic. It should be run before the pre-warmup stage.

Just curious, what's the reason for this? Is it so the diagnostic doesn't interfere with calculating the invokeCount

3 - Now I have troubles with diagnostic of x86 benchmark from the x64 host application. Can we fix it?

Yeah that's a problem with the CLRMD library, it doesn't want to analyse a 32-bit process from a 64-bit host app. I'll take a look, I think I can fix it by having a small shim 32-bit process that can then launch the 32-bit benchmark.

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Nov 30, 2015

Member

Thanks for doing that, at first glance it looks good, but I'll have a more of a look later on today.

Ok. I just implemented basic concepts. Please, refactor the diagnostic classes at its discretion.

Just curious, what's the reason for this?

Theoretically, it can affects the warmup process. We should try to avoid any additional logic between our runs. So, I suggest to create a special stage: diagnostic. We can run it directly without hacks like if (line.StartWith("// Warmup") .... The source code will be more clear.

I'll take a look, I think I can fix it by having a small shim 32-bit process that can then launch the 32-bit benchmark

Sounds cool.

Member

AndreyAkinshin commented Nov 30, 2015

Thanks for doing that, at first glance it looks good, but I'll have a more of a look later on today.

Ok. I just implemented basic concepts. Please, refactor the diagnostic classes at its discretion.

Just curious, what's the reason for this?

Theoretically, it can affects the warmup process. We should try to avoid any additional logic between our runs. So, I suggest to create a special stage: diagnostic. We can run it directly without hacks like if (line.StartWith("// Warmup") .... The source code will be more clear.

I'll take a look, I think I can fix it by having a small shim 32-bit process that can then launch the 32-bit benchmark

Sounds cool.

@mattwarren

This comment has been minimized.

Show comment
Hide comment
@mattwarren

mattwarren Dec 11, 2015

Contributor

Theoretically, it can affects the warmup process. We should try to avoid any additional logic between our runs. So, I suggest to create a special stage: diagnostic.

This is fine, I'll can add an extra step and only run it if we need diagnostics, i.e. BenchmarkState.Instance.IterationMode = BenchmarkIterationMode.Diagnostics;

We can run it directly without hacks like if (line.StartWith("// Warmup") .... The source code will be more clear.

I had a look at this, the main problem is that we are in different processes. The diagnostics are run from the main process, where as BenchmarkIterationMode is set in the process we spawn. So the only real way they have to communicate between the 2 is by looking at the console output, or am I missing something?

Contributor

mattwarren commented Dec 11, 2015

Theoretically, it can affects the warmup process. We should try to avoid any additional logic between our runs. So, I suggest to create a special stage: diagnostic.

This is fine, I'll can add an extra step and only run it if we need diagnostics, i.e. BenchmarkState.Instance.IterationMode = BenchmarkIterationMode.Diagnostics;

We can run it directly without hacks like if (line.StartWith("// Warmup") .... The source code will be more clear.

I had a look at this, the main problem is that we are in different processes. The diagnostics are run from the main process, where as BenchmarkIterationMode is set in the process we spawn. So the only real way they have to communicate between the 2 is by looking at the console output, or am I missing something?

mattwarren added a commit that referenced this issue Jan 4, 2016

More robust version of the Diagnostic Library (see #53)
Will always find it's dependencies (i.e. 'msdia120.dll') when running as
parts of Integration Tests
@mattwarren

This comment has been minimized.

Show comment
Hide comment
@mattwarren

mattwarren May 5, 2016

Contributor

Now being tracked via #160, so closing this old issue

Contributor

mattwarren commented May 5, 2016

Now being tracked via #160, so closing this old issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment