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

[Xamarin.Android.Build.Tasks] don't copy pdb/mdb to $(OutputPath) #2935

Merged
merged 1 commit into from
Apr 9, 2019

Conversation

jonathanpeppers
Copy link
Member

Looking at the build output of a simple app:

> ls .\samples\HelloWorld\bin\Debug\*.pdb
  Length Name
  ------ ----
    1024 HelloLibrary.pdb
    1304 HelloWorld.pdb
13943324 Mono.Android.pdb
  153048 Mono.Security.pdb
 1545052 mscorlib.pdb
  106160 System.ComponentModel.Composition.pdb
  393532 System.Core.pdb
   43076 System.Net.Http.pdb
  795468 System.pdb
  325732 System.Runtime.Serialization.pdb
   77008 System.ServiceModel.Internals.pdb
  917588 System.Xml.pdb

There are not accompanying .dll files for many of these symbols.
Some of these symbols are also quite big!

I don't think we need to be copying these files to $(OutputPath) at
all; it doesn't make sense if their accompanying assembly isn't there!

If we remove the <CopyIfChanged/> calls in our targets, MSBuild will
handle copying the appropriate symbol files that are needed.

After this change, the directory looks like:

> ls .\samples\HelloWorld\bin\Debug\*.pdb
  Length Name
  ------ ----
  1024 HelloLibrary.pdb
  1304 HelloWorld.pdb

Results

I did a performance comparison using the Xamarin.Forms app in this
repo. It seems the extra <CopyIfChanged/> call was taking between
25-50ms, but the bigger savings here is to not copy a 13MB
Mono.Android.pdb file. There could be bigger savings for developers
that have slower hardware (or don't have SSDs).

Overall it seems this change saves up to 50ms for a small project on
initial build, or incremental builds where an assembly changed.

Looking at the build output of a simple app:

    > ls .\samples\HelloWorld\bin\Debug\*.pdb
      Length Name
      ------ ----
        1024 HelloLibrary.pdb
        1304 HelloWorld.pdb
    13943324 Mono.Android.pdb
      153048 Mono.Security.pdb
     1545052 mscorlib.pdb
      106160 System.ComponentModel.Composition.pdb
      393532 System.Core.pdb
       43076 System.Net.Http.pdb
      795468 System.pdb
      325732 System.Runtime.Serialization.pdb
       77008 System.ServiceModel.Internals.pdb
      917588 System.Xml.pdb

There are *not* accompanying `.dll` files for many of these symbols.
Some of these symbols are also quite big!

I don't think we need to be copying these files to `$(OutputPath)` at
all; it doesn't make sense if their accompanying assembly isn't there!

If we remove the `<CopyIfChanged/>` calls in our targets, MSBuild will
handle copying the appropriate symbol files that are needed.

After this change, the directory looks like:

    > ls .\samples\HelloWorld\bin\Debug\*.pdb
      Length Name
      ------ ----
      1024 HelloLibrary.pdb
      1304 HelloWorld.pdb

~~ Results ~~

I did a performance comparison using the Xamarin.Forms app in this
repo. It seems the extra `<CopyIfChanged/>` call was taking between
25-50ms, but the bigger savings here is to not copy a 13MB
Mono.Android.pdb file. There could be bigger savings for developers
that have slower hardware (or don't have SSDs).

Overall it seems this change saves up to 50ms for a small project on
initial build, or incremental builds where an assembly changed.
@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Apr 5, 2019

Does someone need to look at this from the VS for Mac side? Or is that you now @joj?

@jonathanpeppers
Copy link
Member Author

macOS failures are network-related:

Test Result (8 failures / -12)
AndroidClientHandlerTests.Mono.Android_TestsMultiDex, Xamarin.Android.NetTests.AndroidClientHandlerTests.Sanity_Tls_1_2_Url_WithMonoClientHandlerFails / Release
AndroidClientHandlerTests.Mono.Android_TestsMultiDex, Xamarin.Android.NetTests.AndroidClientHandlerTests.Sanity_Tls_1_2_Url_WithMonoClientHandlerFails / Release
AndroidClientHandlerTests.Mono.Android_Tests, Xamarin.Android.NetTests.AndroidClientHandlerTests.Tls_1_2_Url_Works / Release
AndroidClientHandlerTests.Mono.Android_Tests, Xamarin.Android.NetTests.AndroidClientHandlerTests.Tls_1_2_Url_Works / Release
SslTest.Mono.Android_Tests, System.NetTests.SslTest.SslWithinTasksShouldWork / Release
SslTest.Mono.Android_TestsMultiDex, System.NetTests.SslTest.SslWithinTasksShouldWork / Release
SslTest.Mono.Android_Tests, System.NetTests.SslTest.SslWithinTasksShouldWork / Release
SslTest.Mono.Android_TestsMultiDex, System.NetTests.SslTest.SslWithinTasksShouldWork / Release

@jonathanpeppers jonathanpeppers added the do-not-merge PR should not be merged. label Apr 8, 2019
@jonathanpeppers
Copy link
Member Author

I will go do a manual test of this one in both IDEs Windows & Mac.

@jonathanpeppers jonathanpeppers removed the do-not-merge PR should not be merged. label Apr 8, 2019
@jonathanpeppers
Copy link
Member Author

Debugging seems to be fine with this change (in both IDEs).

The only thing I can think of that might possibly break -- if you had a NuGet package via <PackageReference/> and wanted to step into it.

I'm not sure that currently works anyway, as we saw in: #2460 (comment)

@jonpryor jonpryor merged commit e997008 into dotnet:master Apr 9, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants