1 2 3 Previous Next 32 Replies Latest reply on May 23, 2006 11:18 AM by manik

    Performance of Method.equals

    brian.stansberry

      We do a lot of calls to Method.equals() in our interceptors, so I thought I'd take a look to see how efficient it is. Here's the code:

       public boolean equals(Object obj) {
       if (obj != null && obj instanceof Method) {
       Method other = (Method)obj;
       if ((getDeclaringClass() == other.getDeclaringClass())
       && (getName() == other.getName())) {
       if (!returnType.equals(other.getReturnType()))
       return false;
       /* Avoid unnecessary cloning */
       Class[] params1 = parameterTypes;
       Class[] params2 = other.parameterTypes;
       if (params1.length == params2.length) {
       for (int x = 0; x < params1.length; x++) {
       if (params1[x] != params2[x])
       return false;
       }
       return true;
       }
       }
       }
       return false;
       }


      No "this == other" check at the beginning! This means that when two methods are identical you go through the whole check.

      We are using static fields to hold refs to our methods, and with EnhancedTreeCacheMarshaller we can now ensure that the Method object associated with a replicated call also is the same one referred to by the static TreeCache field. So, we can potentially get a performance boost by replacing Method.equals() with x == TreeCache.y.

      The downside to doing that is it is breakable if we somehow introduce code that uses a different object to represent the method. E.g. unmarshalling code for backwards compatibility that just deserializes a method.

      A safer alternative is to add a util method

      // Unsafe because no null check on x
      public static boolean fastUnsafeEquals(Method x, Method y)
      {
       return (x == y || x.equals(y));
      }


      But now, every time x !=y (presumably the vast majority of cases) we've added an extra step. So whether that would be more performant is questionable.

        • 1. Re: Performance of Method.equals
          clebert.suconic

          These references are for users or JBossCache's classes?

          If you are keeping a reference for a user's code this will cause a redeployment leakage.

          • 2. Re: Performance of Method.equals
            genman


            I took a look at hot spots using performance tools. There is a bit of time spent doing Method.equals() calls in all the interceptors. I found I could collapse a lot of the "if" calls using a hashtable. I also noticed that the Method.hashCode only returns a hash of the method name and class name. I don't know if my optimization really mattered much in the end, but since the code was cleaner it was a win in terms of clarity.

            Isn't using AOP going to be the long term solution anyway?

            • 3. Re: Performance of Method.equals
              brian.stansberry

               

              "clebert.suconic@jboss.com" wrote:
              These references are for users or JBossCache's classes?


              These are static fields in the TreeCache class holding references to Method objects that represent its own methods.

              With Manik's marshalling enhancements we no longer serialize these Method objects; we just serialize a byte that tells which method. So when we unmarshall we just use the same Method object referenced by the static field.

              • 4. Re: Performance of Method.equals
                brian.stansberry

                 

                "genman" wrote:

                I took a look at hot spots using performance tools. There is a bit of time spent doing Method.equals() calls in all the interceptors. I found I could collapse a lot of the "if" calls using a hashtable. I also noticed that the Method.hashCode only returns a hash of the method name and class name. I don't know if my optimization really mattered much in the end, but since the code was cleaner it was a win in terms of clarity.


                Clarity is good :) Where did you do this? I randomly picked a few interceptors and didn't see it.

                Isn't using AOP going to be the long term solution anyway?


                Not quite sure what you mean.

                • 5. Re: Performance of Method.equals
                  genman


                  Clarity is good :) Where did you do this? I randomly picked a few interceptors and didn't see it.


                  Actually, I only changed one as an exercise. And it looks like I didn't commit my changes for the PessimisticLockInterceptor class :-( I guess I held off for some reason.

                  Not quite sure what you mean.


                  Instead of passing along the MethodCall, define add/remove pointcuts etc. I've never done AOP myself, but couldn't the interceptors be written so that you could say (concisely) "for all put methods, use a read/write lock. for all get methods, use a read lock"?

                  • 6. Re: Performance of Method.equals
                    brian.stansberry

                     

                    "genman" wrote:

                    Instead of passing along the MethodCall, define add/remove pointcuts etc. I've never done AOP myself, but couldn't the interceptors be written so that you could say (concisely) "for all put methods, use a read/write lock. for all get methods, use a read lock"?


                    Got it. Sorry, brain didn't engage before :). Yes, that makes sense.

                    • 7. Re: Performance of Method.equals

                      While we are on this optimization and benchmark subject, just to let you everyone knows that I have written a simple perf test for TreeCache and PojoCache. It is similar to Bela's JGroups tester.

                      It is located under JBossCache/tests/scripts. There is a readme to explain how to run it. It is focused on simulating http clustering load test (e.g., no write contention) though.

                      It is still going thru enhancement iterations as I am doing the benchmark tests for PojoCache. But suggestion is welcome and I thought this is a good way to measure your performance metrics.

                      • 8. Re: Performance of Method.equals

                        AOP poincut provides you the flexibility of defining where you want to customize your interceptor behaviors (at configuration time). So this is useful for the user of JBossCache to provide their own cutomized solution.

                        But for our internal aspects, I thought the current interceptors have covered it really well, except we need to externalize the configuration of interceptor stack. So move to AOP land will be great but not that urgent, IMO.

                        • 9. Re: Performance of Method.equals
                          manik

                          Re: AOP, it is a nice to have and in no way urgent. As Ben pointed out, the current interceptor architecture suits our purpose quite well, the only motivation to using AOP internally (as far as I can see) is to make the JBossCache codebase cleaner/more readable. Good reasons, for sure, but perhaps not a very high priority.

                          • 10. Re: Performance of Method.equals
                            manik

                            Regarding performance of Method.equals(), how about creating a class of our own - LightweightMethod - that acts as a delegate to Method?

                            The map of 'known' method ids (each id a single byte) that currently exist in EnhancedTreeCacheMarshaller can be moved to LightweightMethod.

                            The TreeCache static block can initialise with static references to LightweightMethods, and LightweightMethod.equals() could compare based on the byte ids.

                            And the multiple if's we have in each interceptor could be replaced with an elegant switch statement, on LightweightMethod.getId().

                            • 11. Re: Performance of Method.equals
                              brian.stansberry

                              I like what you're suggesting, but I guess we'd have to subclass the JGroups MethodCall so we could pass around the LightweightMethod through Interceptor.invoke(MethodCall).

                              Unfortunately, we'd have to use the delegate approach and can't just subclass Method, as it's final.

                              • 12. Re: Performance of Method.equals
                                manik

                                Sorry you're right, MethodCall and not Method. There is no need to subclass or even delegate to Method.

                                So we have a LightweightMethodCall which is a subclass of MethodCall, and maintains a table of known method ids which the EnhancedTreeCacheMarshaller can reference.

                                LightweightMethodCall adds:
                                * a getMethodId() method which returns a byte
                                * a lookupMethod(byte id) method which returns a Method
                                * Exposes static bytes representing known method ids such as LightweightMethodCall.COMMIT_METHOD_ID, etc.

                                Interceptors replace the multiple if's with a switch(methodCall.getMethodId())

                                When creating method calls, we simply replace all instances of new MethodCall() with new LightweightMethodCall().

                                What do you think?

                                • 13. Re: Performance of Method.equals
                                  manik

                                  Now this sounds very simple to do and probably won't take anyone more than a couple of hours to change plus test, but is it worthwhile doing?

                                  Do we have any numbers to tell how much of an overhead the existing code is? I think we should do the above anyway - if only for the sake of readability and clarity - but if we have a compelling performance gain as well I'll move it up in terms of priority in my otherwise rather full stack of tasks for 1.4.0. :-)

                                  Unless, of course, someone else volunteers to take this on. (hint, hint)

                                  • 14. Re: Performance of Method.equals
                                    genman


                                    I did some perf testing and you'll gain maybe 1-2 percent tops for the local cache, single threaded case. Maybe less than 1%.

                                    I would recommend you use "int" not "byte" since "int" is what processors deal with natively.

                                    http://www.liemur.com/Articles/FineTuningJavaCode-IntOrientedMachine.html

                                    I think clarity would probably improve for methods that do conversion. That's more important than performance issues anyway.

                                    1 2 3 Previous Next