Skip to content

Create profiling symbols by default. #1231

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

Merged
merged 1 commit into from
May 19, 2017

Conversation

eerhardt
Copy link
Member

@gkhanna79 @ramarag

/cc @nguerrera

Note: the tests needed to turn this option off because they are using netcoreapp1.0, which errors out when generating profiling pdbs. We don't support dotnet store for netcoreapp1.0, so it makes sense to shut off profiling symbols in those tests. (which is what we were doing before this change)

@eerhardt
Copy link
Member Author

This supports half of #1126 (comment)

@nguerrera
Copy link
Contributor

I'm not seeing the netcoreapp1.0 related changes described in description, only OSX.

Also, do we have a nice error message for trying to use dotnet store on netcoreapp1.0 or does it just blow up?

@eerhardt
Copy link
Member Author

Customer scenario

Currently, when generating a "runtime store" users don't get symbols generated by default. This means they can't profile their crossgen'd assemblies correctly.

This change turns symbol generation on by default.

Bugs this fixes:

Fixes 1 of 2 open issues left in #1126. The other issue needs to be fixed in dotnet/cli.

Workarounds, if any

Users can specify the option themselves, but this isn't our preferred approach. If you generate an assembly, the default should be to be able to profile it.

Risk

Low. There are not many users of this functionality. Just ASP.NET, and they have been notified of this change.

Performance impact

This makes the dotnet store command a bit slower, but this is an advanced command which normal users don't use. Also, this helps users write more performant code, since they can now profile it.

Is this a regression from a previous update?
No

Root cause analysis:
Planned Feature

How was the bug found?
Planned Feature

@MattGertz for approval

@eerhardt
Copy link
Member Author

eerhardt commented May 18, 2017

I'm not seeing the netcoreapp1.0 related changes described in description

It's the 3 tests that have profiling symbols turned off now:

image

Also, do we have a nice error message for trying to use dotnet store on netcoreapp1.0 or does it just blow up?

See #1048. Some things work. A lot don't. The issue asks to just disable it completely.

@nguerrera
Copy link
Contributor

I see. Should we just retarget those tests if the plan is that we'll be not be supporting netcoreapp1.1 with dotnet store by RTM?

@eerhardt
Copy link
Member Author

I see. Should we just retarget those tests if the plan is that we'll be not be supporting netcoreapp1.1 with dotnet store by RTM?

Yes, we will have to do that when we disable TFMs pre-netcoreapp2.0.

@eerhardt eerhardt merged commit ef28455 into dotnet:master May 19, 2017
@eerhardt eerhardt deleted the ProfilingSymbols branch May 19, 2017 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants