Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Documentation (or, is this project dead?) #75

Open
JasonBock opened this issue Aug 18, 2019 · 6 comments
Open

Documentation (or, is this project dead?) #75

JasonBock opened this issue Aug 18, 2019 · 6 comments

Comments

@JasonBock
Copy link

I've been playing around with this package, and while I think it has some merit and potential, it feels woefully under-documented, and its advice is confusing.

For example, I ran across an interpolated string in my code where the analyzer raised HAA0601. The help link does nothing, nor is there any code fix available, so I really don't know how to fix this. I'm guessing it's because the value types I have in the string will get boxed? So I created a small benchmark project:

using BenchmarkDotNet.Attributes;

namespace TestFormatting
{
	[MemoryDiagnoser]
	public class FormattingAndBoxing
	{
		private readonly uint value1 = 324;
		private readonly int value2 = 43219;

		[Benchmark]
		public int FormatStringWithNoToString() =>
			$"The expected call count is incorrect. Expected: {this.value1}, received: {this.value2}.".Length;

		[Benchmark]
		public int FormatStringWithToString() =>
			$"The expected call count is incorrect. Expected: {this.value1.ToString()}, received: {this.value2.ToString()}.".Length;
	}
}

The results were....interesting:

|                     Method |     Mean |    Error |   StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------- |---------:|---------:|---------:|-------:|------:|------:|----------:|
| FormatStringWithNoToString | 400.7 ns | 4.240 ns | 3.759 ns | 0.0496 |     - |     - |     208 B |
|   FormatStringWithToString | 118.4 ns | 1.277 ns | 1.132 ns | 0.0687 |     - |     - |     288 B |

So the second way is faster, but it actually allocates a bit more memory? Is this correct? If so, how do I really address this analyzer's diagnostic? Or is my test faulty?

It's really down to prescriptive advice. This package needs far better documentation for it to be useful on every-day projects. I'd love to use it and continue to provide feedback, but it's unclear if this is even being maintained anymore.

@mjsabby
Copy link
Contributor

mjsabby commented Sep 14, 2019

The project is not dead ... but at the same time the Roslyn team has decided they are going to incorporate this into some sort of Roslyn tooling. @jinujoseph / @sharwell?

@sharwell
Copy link
Member

The Roslyn team forked this code to dotnet/roslyn-analyzers with the intent of working towards a production-ready set of performance analyzers. Allocations are only one source of observed performance problem. Ideally, we want users to be able to document "performance sensitive" sections of code with [PerformanceSensitive], which links to a documented measurement scenario and describes the constraints that developers working in the code need to be aware of. Where possible, the analyzers would help enforce those constraints during builds.

@stebet
Copy link
Contributor

stebet commented Oct 9, 2019

Is there a preview of these performance analyzers yet?

@sharwell
Copy link
Member

sharwell commented Oct 9, 2019

@stebet The analyzers are getting published here: https://dotnet.myget.org/feed/roslyn-analyzers/package/nuget/Microsoft.CodeAnalysis.PerformanceSensitiveAnalyzers

The issue tracker for that package is here: https://github.com/dotnet/roslyn-analyzers/issues

@GitClickOk
Copy link

I agree with @JasonBock. I loved this package, but even for simple things can be hard to tell what's wrong or what we can do about it. For example:

    public class Foo
    { 
        public int Id { get; set; }
    }

    public class Bar
    {
        void DoSomethingWithFoo(Foo someFoo)
        {
            var somethingWithFoo = $"We received the Foo Id {someFoo.Id}";
        }
    }

We received this warning:

HAA0601 Value type to reference type conversion causes boxing at call site (here), and unboxing at the callee-site. Consider using generics if applicable

and... what? I can't imagine a simpler code, and for some reason we need change or fix this code. A really really simple suggestion about what's happening or what we can do will be really amazing.

@JasonBock
Copy link
Author

So....did this ever get "moved"/"merged" to dotnet/roslyn-analyzers? I've looked at the site and I can't find any information on allocation analyzers, but I might just be missing it. If it has been moved there, I can close this comment.

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

No branches or pull requests

5 participants