Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      The Rules that invoke methods; CallMethodRule, SetNextRule, SetRootRule,
      SetTopRule use a method in beanutils.MethodUtils which does a lookup of the
      method each and everytime it is invoked, obviously this can greatly impact the
      performance of the Digester.

      My suggestion is to have the specific Rule instance do a lookup of the method
      upon first invokation and keep the reference to the Method in the Rule instance
      for later re-use.

      I have made this modification locally and have observed typical Digester speed
      improvements of 20-50% depending on the specific Digester rules.

        Activity

        Hide
        rdonkin@apache.org added a comment -

        thanks for posting this to bugzilla, greg, and thanks for the figures.

        (i had a feeling that your patch would turn out to be this.)

        i've thought about this before and came to the conclusion that generic caching the right
        way to solve this problem. by this i mean that the caching should be done in the reflection
        layer rather than in the calling component. that way, every component using the
        reflection library can enjoy this advantage, not just digester.

        i didn't realise that method caching made such a large difference (but i'm on java 1.3 and
        so reflection is s l o w) and i'll try to raise this issue as soon as lang is released. please stick
        around and join in the discussions.

        • robert
        Show
        rdonkin@apache.org added a comment - thanks for posting this to bugzilla, greg, and thanks for the figures. (i had a feeling that your patch would turn out to be this.) i've thought about this before and came to the conclusion that generic caching the right way to solve this problem. by this i mean that the caching should be done in the reflection layer rather than in the calling component. that way, every component using the reflection library can enjoy this advantage, not just digester. i didn't realise that method caching made such a large difference (but i'm on java 1.3 and so reflection is s l o w) and i'll try to raise this issue as soon as lang is released. please stick around and join in the discussions. robert
        Hide
        Greg Messner added a comment -

        When I was profiling the Digester code I was suprised that the
        beanutils.MethodUtils didn't cache the looked up methods.

        But in the case of the Digester Rules, I'd rather keep just the Method reference
        than the name of the method around for later lookup, because doing a method
        lookup is more than just looking up a method by name, it also requires you to
        build up a Class[] to specifiy the parameters for the method.

        As often as the Digester does method invokation I think it makes sense to cache
        the Method reference in the specific Digester Rule instance, because to some of
        us every bit of speed helps.

        That being said, I do think it would be a good idea for the lang package to
        cache things that are looked up.

        Show
        Greg Messner added a comment - When I was profiling the Digester code I was suprised that the beanutils.MethodUtils didn't cache the looked up methods. But in the case of the Digester Rules, I'd rather keep just the Method reference than the name of the method around for later lookup, because doing a method lookup is more than just looking up a method by name, it also requires you to build up a Class[] to specifiy the parameters for the method. As often as the Digester does method invokation I think it makes sense to cache the Method reference in the specific Digester Rule instance, because to some of us every bit of speed helps. That being said, I do think it would be a good idea for the lang package to cache things that are looked up.
        Hide
        rdonkin@apache.org added a comment -

        the advantage of caching in the reflection layer is that the cache can very easily be shared
        between all rule instances.

        if the cache is written correctlty, it will cache at every stage - it will cache methods[] calls as
        well as mapping method to parameter for use in invocation. getting methods[] is very
        slow and caching both will improve typical usage much more than caching in a single rule.

        the right place to implement these caching mechanisms is in the reflection layer rather
        than in digester.

        Show
        rdonkin@apache.org added a comment - the advantage of caching in the reflection layer is that the cache can very easily be shared between all rule instances. if the cache is written correctlty, it will cache at every stage - it will cache methods[] calls as well as mapping method to parameter for use in invocation. getting methods[] is very slow and caching both will improve typical usage much more than caching in a single rule. the right place to implement these caching mechanisms is in the reflection layer rather than in digester.
        Hide
        Greg Messner added a comment -

        I'm not talking about caching in the Digester, I'm simply talking about keeping
        a reference to the Method instance in the Rule vs. keeping the String of the
        method name or a String with the fully specified method signature which would
        be required to do the lookup in the cache.

        Implement the caching in the lang package where it belongs but don't force a
        consumer of the package to use the cache. As a consumer of the lang package if
        I want to keep a reference to the Method around the lang package should not
        prohibit this.

        Show
        Greg Messner added a comment - I'm not talking about caching in the Digester, I'm simply talking about keeping a reference to the Method instance in the Rule vs. keeping the String of the method name or a String with the fully specified method signature which would be required to do the lookup in the cache. Implement the caching in the lang package where it belongs but don't force a consumer of the package to use the cache. As a consumer of the lang package if I want to keep a reference to the Method around the lang package should not prohibit this.
        Hide
        Craig McClanahan added a comment -

        While I agree that the suggested change could be useful, it would be better to
        defer this change until the commons-lang version of MethodUtils is finished and
        released. The current commons-beanutils version of MethodUtils is poorly
        factored, and would require the commons-digester Rule implementations to have
        more knowledge of the internals of how MethodUtils does introspection than is
        really appropriate.

        Therefore, I'm marking this as RESOLVED/LATER so that it's not forgotten.

        Show
        Craig McClanahan added a comment - While I agree that the suggested change could be useful, it would be better to defer this change until the commons-lang version of MethodUtils is finished and released. The current commons-beanutils version of MethodUtils is poorly factored, and would require the commons-digester Rule implementations to have more knowledge of the internals of how MethodUtils does introspection than is really appropriate. Therefore, I'm marking this as RESOLVED/LATER so that it's not forgotten.
        Hide
        rdonkin@apache.org added a comment -

        digester 1.4 released

        Show
        rdonkin@apache.org added a comment - digester 1.4 released
        Hide
        rdonkin@apache.org added a comment -

        after 1.4.1 release

        Show
        rdonkin@apache.org added a comment - after 1.4.1 release
        Hide
        rdonkin@apache.org added a comment -

        digester 1.4.1 released

        Show
        rdonkin@apache.org added a comment - digester 1.4.1 released
        Hide
        Mohan Kishore added a comment -

        Have opened an enhancement request against lang, and attached a patch for
        caching methods in MethodUtils.

        http://nagoya.apache.org/bugzilla/show_bug.cgi?id=18773

        Would this resolve the problem here?

        Show
        Mohan Kishore added a comment - Have opened an enhancement request against lang, and attached a patch for caching methods in MethodUtils. http://nagoya.apache.org/bugzilla/show_bug.cgi?id=18773 Would this resolve the problem here?
        Hide
        Mohan Kishore added a comment -

        My mistake, I had initially patched the MethodUtils in the lang project. Have
        now patched the MethodUtils in BeanUtils (the one used in Digester):
        http://nagoya.apache.org/bugzilla/show_bug.cgi?id=18960

        Think this should resolve the reported issue...

        Show
        Mohan Kishore added a comment - My mistake, I had initially patched the MethodUtils in the lang project. Have now patched the MethodUtils in BeanUtils (the one used in Digester): http://nagoya.apache.org/bugzilla/show_bug.cgi?id=18960 Think this should resolve the reported issue...
        Hide
        Craig McClanahan added a comment -

        I'm assuming that the fix for Bugzilla #18960 (referenced in the dialog below)
        has addressed this issue sufficiently to close this issue. Please re-open if
        that is not the case.

        Show
        Craig McClanahan added a comment - I'm assuming that the fix for Bugzilla #18960 (referenced in the dialog below) has addressed this issue sufficiently to close this issue. Please re-open if that is not the case.

          People

          • Assignee:
            Unassigned
            Reporter:
            Greg Messner
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development