Warning CA1822 : Microsoft.Performance :

Warning CA1822 : Microsoft.Performance :

• RamiKhalyleh

  Just to be clear, we are not recommending that all methods be marked 'Shared'. There are some methods which simply cannot be marked shared because they use 'Me', access non-shared fields or call other non-shared methods. However, if a method does none of these things, it can and should be marked 'Shared'.

  In your case, it sounds like you have an entire class consisting of helper methods which do not depend on the class being instantiated (e.g. Dim myFormClass as new ActivateFormClass()). This is often referred to as a 'static helper class'. Those particular classes normally do have all of their methods marked as 'Shared' and that is exactly what the rule is recommending in this case.

  Thanks!
  Nick

  
  

• Akhlesh

  Hi David
  
  The two points you made in bullets are the points I made... which I agree with and why I think the rule is valuable but shouldn't be in the performance category.
  
  As for the issues you mention with the test, (1) I'm not trying to demonstrate 'real' usage, I'm trying to show the effects that that different types of method call make on performance. The best way to show this is by calling an empty method which cannot affect the amount of time spent in the method itself thus the differences are from the actual method call time (2) There is no class initializer to run as there is no static constructor, the effect of loading the class if it is a static will be pretty irrelevant anyway - a few clock cycles at most (3) Testing callvirt against call isn't moot, it shows that the most important performance factor with types of method call is the compiler's choice of instruction and not whether you make it static or instance, further supporting my case that the choice between instance and static methods has nothing to do with performance.
  
  You're right though, it would be better to call each method once before the measurement to get all of the JIT and initialization out of the way.
  
  As I said, the rule does have merit, but it has nothing to do with performance.
  
  Greg
  
  
  

• JJK

  Greg,

  First of all before we talk about the performance difference, there are a number of other reasons this rule exists:

  • Making a method static makes it clear on its intention not to touch instance state
  • The rule could indicate a bug in your code (similar to ReviewUnusedParameters) by your failure to touch any instance state

  The reason this rule is under performance, is because (currently) a rule can only exist under one category. It used to be combined with the ReviewUsedParameters rule (under Microsoft.Usage), however, users were confused when they encountered a warning stating that the 'this' parameter was unused, hence why it was moved into its own rule.

  I've had a look at your test case, and unfortunately it has a couple of issues; (1) you are not measuring against against real data (calling empty methods millions of times isn't real usage), (2) you are including only the JIT compilation in the first test (instance), but both the JIT compilation and class initialization (by the CLR) time in the second test (static), (3) the test measuring the performance gain by using a 'call' over a 'callvirt' is moot as the C# compiler always uses 'callvirt' to call both virtual and non-virtual instance methods.

  As a fairer measurement, try calling each method once before you start measuring. When I did that, I found that the static method was 1% faster*. A modest improvement, but when combined with the above reasons, I think this rule still has merit.

  * There are a couple of reasons why this would be, (1) there is not an implicit null check on the 'this' pointer (via the callvirt call), (2) the 'this' pointer does not have to be pushed on the stack as a parameter. 

  Regards

  David

  
  
  

• Yahweh

  Ok but no matter where i move it or any of the other function for that matter, now they all have to be
  

  Public Shared Sub CheckActive(byval formcheck as form)

  Becuae no matter where i stick any of the sub or functions i am getting this error

   

  Thanks,

  Joe

  
  

• moteris

  Greg,

  Thanks for responding.

  First of all, while the declaring type of the static method did not have a static constructor, the initializing of the static class (or loading) was obviously having an effect on performance, as indicated by your numbers.

  The Visual Basic and C# Compilers will always output a callvirt when calling a normal instance method (except when calling a base method via base) and call for static members, so I'm a little confused by this statement 'further supporting my case that the choice between instance and static methods has nothing to do with performance', as it obviously slower to make a callvirt call (as the method has to be resolved at runtime) over a normal call.

  I should also add that most of FxCop's performance rules have orginated after issues were found in real-world managed applications developed by Microsoft. However, in saying, the rules should be taken in context of a entire application, and a performance problem found in one application, does not mean it will be a problem in another.

  In fact, most of the performance rules are not in the MinBar (a group of rules that must be addressed for all code) for the entire Visual Studio, .NET Framework and WinFx teams.

  Regards

  David

  
  
  

• DanielLei

  The only time I've ever ignored this warning is when the methods are in a MarshalByRef class that I'm using to communicate between AppDomains. In this case the method has to be left as an instance method.

  Like I said in earlier posts, this is a great rule that points out that you were probably thinking wrong when you created the method...

  
  

• Vaish

  I agree on the fact that methods never using this/Me should be marked static/Shared, especially for static helper classes.

  However, in my case I code in C# and use multi-threading. Isn't it a bit dangerous if 2 instances of whatever objects call simultaneously the same static method of a helper class It would mean that both objects call a method that will share the same memory space for both calls, and might lead to interference. Am I wrong somewhere If not, should I keep my helper classes as non-static

  thx

  
  

• nerd_bomber

  Making the method Shared should have stopped the FxCop warning. You implied that it didn't so that doesn't make any sense.

  Your question at the end was:

  So would this be the way they want you to use classes now

  I would answer by saying that if you have a method in a class that doesn't use anything from the classes instance, then it should be marked as Shared. If you understand "instance" and "Shared" then this should be fairly obvious. I'm not trying to talk down to you, I just trying to figure out what you are asking.

  If I was doing what I think you are trying to do I would make a class called FormHelper. I would find a better name than CheckActiveMidChild since it has the side effect of bringing the existing form to the front or creating a new form. Neither of those side effects are obvious from the current method name. Still, I'm not trying to talk down to you, I'm just very anal about naming things well so others can use them and maintain them...

  
  

• zigy42

  Class ActivateFormClass

  'Check to see if form is open already if so bring it to the front ***

  Public Shared Sub CheckActiveMidChild(ByVal formCheck As Form)

  Dim frmMdiParent As Form = BmcForm

  Dim frmMdiChild As Form

  Try

  If formCheck Is Nothing Then

  Else

   

  'Check to see if the form is open already

  For Each frmMdiChild In frmMdiParent.MdiChildren

  If frmMdiChild.Equals(formCheck) Then

  formCheck.BringToFront()

  Exit Sub

  End If

  Next

  'No Form lets open it

  formCheck.MdiParent = frmMdiParent

  formCheck.Activate()

  formCheck.Show()

  End If

  Catch ex As Exception

  My.Application.Log.WriteException(ex, TraceEventType.Error, " ActivateForm_class - CheckActiveMidChild ")

  Finally

  ' Me.Finalize()

  End Try

  End Sub

  End Class
  
  
  
  ** So would this be the way they want you to uses classes now then
  Thanks,
  Joe

  
  

• wyzec

  The warning is actually correct. You never use "Me" throughout your method. It's kind of odd to think about but instance (not shared) methods actually get a hidden parameter which is an instance of the class. In other words, your method is really:

  Public Sub CheckActiveMidChild(Me As Xxx, ByVal formCheck As Form)

  The first parameter is just "hidden". If you make the method Shared, then the hidden parameter won't be passed and you will save yourself something like 1 billionth of a second over the next 5000 years.

  I really like this rule, not for the performance implications, but because it makes me rethink why I made the method an instance method in the first place. Basically the method doesn't use anything from the instance of the class its in so it really should be Shared. Sometimes this points out that this is really a utility or helper method that should be put into something like a FormHelper utility class.

  
  

• kd4gar

  So now if you have say 4 functions setting in a class that just return values independently from the class.

  Public Class Form1

  Public Function GetInfo(ByVal I As Int16) As SqlDataReader

  Hit(DB)

  Return Data

  End Function

  
  ----------- Should be done in 2.0 lusing fxcop ike this

  Class Form1

  Public Shared Function GetInfo(ByVal I As Int16) As SqlDataReader

  Hit(DB)

  Return Data

  End Function

  I ask this becuase if you leave the Shared on the Public Class Form1
  Fxcop throws a warning telling you not to do this now
  
  Thanks,
  Joe

  
  

• raviv762

  I think this rule is badly categorised as in my own testing it actually is marginally quicker to make a non-virtual call to an instance method than a call to a static method (though the difference is in the order of clock cycles so is frankly irrelevant except in a very few cases). Here's the quick test I did and the results if you're interested: http://www.gregbeech.com/Blog/Entry.aspx b=1&id=18
  
  Putting it in the Microsoft.Performance category gives the wrong message about why the method maybe should be changed. Personally I think the rule can be useful to make you think about whether the method would make more sense as a static method, but it should be reclassified out of the performance category and the severity/certainty should be dropped to make it a suggestion rather than a warning.
  
  
  
• Warning CA1822 : Microsoft.Performance :