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

Does this work with 2017 now? #33

Open
sebas77 opened this issue Dec 2, 2017 · 20 comments
Open

Does this work with 2017 now? #33

sebas77 opened this issue Dec 2, 2017 · 20 comments

Comments

@sebas77
Copy link

sebas77 commented Dec 2, 2017

If so, how to install it?

@abergs
Copy link

abergs commented Dec 4, 2017

I'm wondering this too

@edespong
Copy link
Contributor

Downloading and installing v1.5 from https://github.com/mjsabby/RoslynClrHeapAllocationAnalyzer/releases is working for me on VS 2017 (15.5)

@niemyjski
Copy link

I think this project is dead?

@sebas77
Copy link
Author

sebas77 commented Dec 22, 2017

1.5 appears to work. This projects shouldn't really die as there is no free alternative (Resharper is the only alternative solution) and is quite fundamental for a coder who values optimizations.

@MarcoRossignoli
Copy link

MarcoRossignoli commented Dec 27, 2017

This projects shouldn't really die

@sebas77
Copy link
Author

sebas77 commented Dec 27, 2017

it has been confirmed that it still in progress by the author. Now I need https://github.com/segrived/Msiler view back as well!

@niemyjski
Copy link

@sebas77 where do you see it's still in progress by the author(s)?

@sebas77
Copy link
Author

sebas77 commented Dec 27, 2017 via email

@mjsabby
Copy link
Contributor

mjsabby commented Mar 11, 2018

Sorry for the lack of progress here, we're trying to find a team that can own this so we can make it better. But for now I'm going to power through and get an update out with fixes that have gone through.

@jlennox
Copy link

jlennox commented Mar 11, 2018

@mjsabby If you have some clear ideas on what needs to be done, I can contribute some time each week. I can be contacted through jlennox at g-mail. Thanks for the work!

edit: I made a fast project to experiment with https://github.com/jlennox/DisposableVisualizer

@MarcoRossignoli
Copy link

MarcoRossignoli commented Mar 12, 2018

@mjsabby i'm not a Roslyn expert yet, but as @jlennox i could help in my spare time, let me know. Thank's for great work!With span revolution advices will be very useful.

@phillijw
Copy link

phillijw commented Jul 6, 2018

Hey all, just wondering how the latest work has been going. Anyone have any updates on when to expect a new version? Nuget package is over 2 years old now :(

@jlennox
Copy link

jlennox commented Jul 6, 2018

@phillijw I've considered making a fork of this project but I'm not sure what needs to be fixed. What are the main issues you're facing, or features you feel are missing?

@ArtGangsta
Copy link

I think what's missing, without which some of us cannot even use the product, is the fact that such a product assumes that you want to know about heap allocations all the time in every piece of code.

That is not the case. It's not like other rules, like, you forgot to call Dispose, or something, which you want to catch every violation of. Heap allocation is a normal thing in C#, so flagging it is kinda useless unless it's being allocated in an area of code that you care about.

What you want is to mark certain methods with an attribute and only check for heap allocations in those methods, and in those cases, actually mark the code with an error instead of a warning, so people don't ignore it.

Or, the attribute should indicate a count, the number of times allocations are expected to occur, and if the code is modified such that new allocations will occur, then it will trigger. That way if you have code with a hot-spot that's taking say 50 allocations, and you work through to minimize it down to say 5, but that's all that you can do.. there's no way to reduce it further, or you are satisfied with the number, then you should be able to say "5 is the minimum for this method", and do so by indicating it in an attribute parameter above the method that this tool will check for.

@jlennox
Copy link

jlennox commented Jul 8, 2018

@ArtGangsta I like those thoughts. No promises as I'm low on free time, but I'll keep this on my mental back burner and poke at it some weekend.

@sebas77
Copy link
Author

sebas77 commented Jul 9, 2018 via email

@edespong
Copy link
Contributor

@ArtGangsta There is a working version in my fork for rougly what you want, here. It is tracked by the issue #7 in this repo, but is dependent on two other pull requests being accepted.

As I have not found any fitting attribute in class libraries, I think that the attribute needs to be configurable in some way. The alternative is that we create a new package with just the attribute which can then be referenced by the projects that needs it.

The branch does currently not solve the "count-allowed-per-method", but I think that could be quite trivial to solve.

@jlennox
Copy link

jlennox commented Jul 11, 2018

@edespong I imagined it would be handled with a pragma or pragma-like comment (which is something Resharper does).

Example of a Resharper "pragma-like" hanging out with pragmas:

#pragma warning disable IDE0007 // Use implicit type
#pragma warning disable IDE1006 // Naming Styles
// ReSharper disable once CheckNamespace

Some thoughts:
Syntax to disable/enable checking in a file:

// AllocationAnalyzer disable
// AllocationAnalyzer enable

Syntax to set/remove a limit between two points:

// AllocationAnalyzer limit 10
// AllocationAnalyzer limit disable

I'm not sure how easy it would be to associate your current analysis between two comments.

@mjsabby
Copy link
Contributor

mjsabby commented Jul 12, 2018

I was thinking of combining a few things:

  1. Have settings in VS for the developer experience
  2. Provide code-level hints as alluded to in [HotPath] attribute to control which methods are analysed #7

For 2. I have the following idea:

    using System;

    [AttributeUsage(AttributeTargets.Constructor | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = true, Inherited = false)]
    public class PerformanceContractAttribute : Attribute
    {
        public DiagnosticType Diagnostic { get; set; }

        public AllocationKind AllowedAllocations { get; set; }
    }

    public enum DiagnosticType
    {
        Warning,
        Error,
        Notify
    }

    [Flags]
    public enum AllocationKind
    {
        None,
        Boxing,
        ImplicitArrayAllocation,
        Captures,
        Explicit,
        Enumerator,
        Delegate,
        All = Boxing | ImplicitArrayAllocation | Captures | Explicit | Enumerator | Delegate // useful when the analyzer setting is set to complain about everything and you still need an escape hatch
    }

    public class Foo
    {
        [PerformanceContract(AllowedAllocations = AllocationKind.Explicit | AllocationKind.Delegate)]
        public void RegularMethod()
        {
        }


        [PerformanceContract(AllowedAllocations = AllocationKind.None, Diagnostic = DiagnosticType.Error)]
        public void SuperHotMethod()
        {
        }

        [PerformanceContract(AllowedAllocations = AllocationKind.All)]
        public void InitializationOrStartupMethod()
        {
        }
    }

When coupled with @edespong's VS options feature we can basically let the user decide/override. However when an attribute on a method is present it could be used in VS AND in the build system as part of continuous integration.

So for example in a csproj, you would say:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <HeapAllocationAnalyzer>PerformanceContract</HeapAllocationAnalyzer>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="ClrHeapAllocationAnalyzer" / Version="1.*" />
  </ItemGroup>

</Project>

What do you all think?

@edespong
Copy link
Contributor

I like the principle of your proposal, msjabby, better than jlennox's. It also solves one of my main issues with just having it in the options, and that is the ability to configure for use in a CI pipeline.

I do, however, think that there is a value in jlennox point about having an accepted baseline of x allocations, which could either be solved by an int on PerformanceContractAttribute, making it very explicit with an int for every allocation kind, or possibly some other way.
An alternative could be to not do anything and force the user to explicitly ignore all known allocations.

I do not know if it is desirable to be able to specify Diagnostic per AllocationKind, something that is not considered above, but I do not think that it is a dealbreaker.

I think that most of the code needed for the proposal is already scattered around a number of branches in my fork, so it should be relatively easy to get a working proposal up and running. If no one else grabs this, I will take a stab when I get back to a proper development environment in middle August.

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

9 participants