Velocity
  1. Velocity
  2. VELOCITY-453

[PATCH] Fix IntrospectionCacheData caching

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None

      Description

      The old code used Class[].hashCode() in MethodCacheKey.hashCode() implementation.
      hashCode() is not overriden for arrays so it returns different value for each array instance.

      The attached is the correct implementation and a test case to prove the caching actually works.

      1. IntrospectionCacheDataTest-v4.java
        1 kB
        Alexey Panchenko
      2. IntrospectionCacheDataTest-no-copyright.java
        1 kB
        Alexey Panchenko
      3. IntrospectionCacheDataTest-no-copyright.java
        1 kB
        Alexey Panchenko
      4. ASTMethod.java.patch2006-09-28
        2 kB
        Alexey Panchenko
      5. ASTMethod_IntrospectionCacheData_cache-v2-retry.patch
        3 kB
        Alexey Panchenko
      6. ASTMethod_IntrospectionCacheData_cache-v2.patch
        3 kB
        Alexey Panchenko
      7. ASF.LICENSE.NOT.GRANTED--IntrospectionCacheDataTest.java
        2 kB
        Alexey Panchenko
      8. ASF.LICENSE.NOT.GRANTED--ASTMethod_IntrospectionCacheData_cache
        3 kB
        Alexey Panchenko

        Activity

        Hide
        Henning Schmiedehausen added a comment -

        Close all resolved issues for Engine 1.5 release.

        Show
        Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.
        Hide
        Alexey Panchenko added a comment -

        The hash function could be as follows

        public int hashCode()
        {
        int result = 17;
        for (int i = 0; i < params.length; ++i)
        {
        final Class param = params[i];
        if (param != null)

        { result = result * 37 + param.hashCode(); }

        }
        return result * 37 + methodName.hashCode();
        }

        17 and 37 are used in HashCodeBuilder by default.

        Show
        Alexey Panchenko added a comment - The hash function could be as follows public int hashCode() { int result = 17; for (int i = 0; i < params.length; ++i) { final Class param = params [i] ; if (param != null) { result = result * 37 + param.hashCode(); } } return result * 37 + methodName.hashCode(); } 17 and 37 are used in HashCodeBuilder by default.
        Hide
        Will Glass-Husain added a comment -

        applied this patch.

        I'm a little shocked that this was broken. Though a subtle bug, it's a fundamental part of template processing. Many thanks for catching this and contributing a patch and unit test. Nicely done.

        r449347

        Show
        Will Glass-Husain added a comment - applied this patch. I'm a little shocked that this was broken. Though a subtle bug, it's a fundamental part of template processing. Many thanks for catching this and contributing a patch and unit test. Nicely done. r449347
        Hide
        Will Glass-Husain added a comment -

        compiling with 1.4 is ok. (must run on 1.3).

        I'm averse to bringing in asserts since they are nowhere else in the code base.

        Show
        Will Glass-Husain added a comment - compiling with 1.4 is ok. (must run on 1.3). I'm averse to bringing in asserts since they are nowhere else in the code base.
        Hide
        Alexey Panchenko added a comment -

        >Do you have hard numbers for that claim? Are these checks used on a hot path? Just curious.
        I can claim without any numbers that code instantiating objects and doing reflection operations will be slower than the code which just compares the object fields. And yes, this code is on the hot path. ASTMethod calls it during template merge. I think everybody wants his template to merge quicker.

        > equals should at least do a if (o == this) return true; check. This short cuts the equals() method in case you test against yourself (set insert e.g.)
        This object is never compared to itself. One first object is used as a key in the HashMap and new objects are instantiated every time to find the cached value.

        > I'd love to have unit tests for hashCode() and equals().
        This class was in code base before my changes. How it comes it was added without unit tests? Just curious.
        To be serious: It is inner class with package visibility. What package should the unit tests be in?

        > There is a potential NPE with params == null in hashCode() and equals()
        This object is never instantiated with params == null. I'd rather add "assert params != null" to the code (if 1.4 syntax is allowed in codebase).

        Show
        Alexey Panchenko added a comment - >Do you have hard numbers for that claim? Are these checks used on a hot path? Just curious. I can claim without any numbers that code instantiating objects and doing reflection operations will be slower than the code which just compares the object fields. And yes, this code is on the hot path. ASTMethod calls it during template merge. I think everybody wants his template to merge quicker. > equals should at least do a if (o == this) return true; check. This short cuts the equals() method in case you test against yourself (set insert e.g.) This object is never compared to itself. One first object is used as a key in the HashMap and new objects are instantiated every time to find the cached value. > I'd love to have unit tests for hashCode() and equals(). This class was in code base before my changes. How it comes it was added without unit tests? Just curious. To be serious: It is inner class with package visibility. What package should the unit tests be in? > There is a potential NPE with params == null in hashCode() and equals() This object is never instantiated with params == null. I'd rather add "assert params != null" to the code (if 1.4 syntax is allowed in codebase).
        Hide
        Henning Schmiedehausen added a comment -

        attachments seem to work now. Henri has discovered that you uploaded your files with trailing blanks on the file names and that confused JIRA. Please don't use trailing blanks on file names (I consider this a JIRA bug BTW).

        Some comments on your patches (I checked the v2-retry.patch because it has the feather):

        • You seem to use some Windows tool. The diffs have ^M line endings for the diff generated lines.
        • equals should at least do a if (o == this) return true; check. This short cuts the equals() method in case you test against yourself (set insert e.g.)
        • I'd love to have unit tests for hashCode() and equals().
        • There is a potential NPE with params == null in hashCode() and equals()
        Show
        Henning Schmiedehausen added a comment - attachments seem to work now. Henri has discovered that you uploaded your files with trailing blanks on the file names and that confused JIRA. Please don't use trailing blanks on file names (I consider this a JIRA bug BTW). Some comments on your patches (I checked the v2-retry.patch because it has the feather): You seem to use some Windows tool. The diffs have ^M line endings for the diff generated lines. equals should at least do a if (o == this) return true; check. This short cuts the equals() method in case you test against yourself (set insert e.g.) I'd love to have unit tests for hashCode() and equals(). There is a potential NPE with params == null in hashCode() and equals()
        Hide
        Henning Schmiedehausen added a comment -

        Do you have hard numbers for that claim? Are these checks used on a hot path? Just curious.

        Show
        Henning Schmiedehausen added a comment - Do you have hard numbers for that claim? Are these checks used on a hot path? Just curious.
        Hide
        Alexey Panchenko added a comment -

        The reason not to use the EqualsBuilder and HashcodeBuilder is the performance.

        Show
        Alexey Panchenko added a comment - The reason not to use the EqualsBuilder and HashcodeBuilder is the performance.
        Hide
        Henning Schmiedehausen added a comment -

        As we have commons-lang as a runtime requirement anyway, is there any reason not to use EqualsBuilder and HashcodeBuilder?

        http://jakarta.apache.org/commons/lang/api-release/org/apache/commons/lang/builder/EqualsBuilder.html
        http://jakarta.apache.org/commons/lang/api-release/org/apache/commons/lang/builder/HashCodeBuilder.html

        I use these all the time in my code and these are very stable and mature classes. No more hand-rolling of equals() and hashCode() for me...

        Show
        Henning Schmiedehausen added a comment - As we have commons-lang as a runtime requirement anyway, is there any reason not to use EqualsBuilder and HashcodeBuilder? http://jakarta.apache.org/commons/lang/api-release/org/apache/commons/lang/builder/EqualsBuilder.html http://jakarta.apache.org/commons/lang/api-release/org/apache/commons/lang/builder/HashCodeBuilder.html I use these all the time in my code and these are very stable and mature classes. No more hand-rolling of equals() and hashCode() for me...
        Hide
        Alexey Panchenko added a comment -

        Right.
        You can replace the header.

        Show
        Alexey Panchenko added a comment - Right. You can replace the header.
        Hide
        Nathan Bubna added a comment -

        Odd. I can only download patchs #2 and #7.

        2. ASTMethod_IntrospectionCacheData_cache-v2-retry.patch
        7. IntrospectionCacheDataTest.java

        Those do at least appear to be the bulk of the code. Alexey, from your comment, i'm guessing you don't care if we just remove the copyright header and add ASL 2 to the test class ourselves, right?

        Show
        Nathan Bubna added a comment - Odd. I can only download patchs #2 and #7. 2. ASTMethod_IntrospectionCacheData_cache-v2-retry.patch 7. IntrospectionCacheDataTest.java Those do at least appear to be the bulk of the code. Alexey, from your comment, i'm guessing you don't care if we just remove the copyright header and add ASL 2 to the test class ourselves, right?
        Hide
        Alexey Panchenko added a comment -

        in the equals method it is correct to use "if (params[i] != other.params[i])" since the params[i] are instanceof Class which does not overide equals()

        Show
        Alexey Panchenko added a comment - in the equals method it is correct to use "if (params [i] != other.params [i] )" since the params [i] are instanceof Class which does not overide equals()
        Hide
        Alexey Panchenko added a comment -

        Sorry, the header was autogenerated by Eclipse and I did not notice it.

        Show
        Alexey Panchenko added a comment - Sorry, the header was autogenerated by Eclipse and I did not notice it.
        Hide
        Will Glass-Husain added a comment -

        Hi Alexey,

        Looks great, thanks for submitting. Appreciate your attention to detail with code conventions as well, makes it easier to read.

        Two minor comments.

        (1) in the equals method, shouldn't the line "if (params[i] != other.params[i])" use the equals method instead?

        (2) Sorry to bug you on this, but we need you to resubmit "IntrospectionCacheDataTest.java" without the copyright notice. You still retain ownership of the code, but copyright notices need to go in a project-wide NOTICES file.

        (more info at: http://www.apache.org/legal/src-headers.html)

        When you have a chance to resubmit, I'll commit.

        Show
        Will Glass-Husain added a comment - Hi Alexey, Looks great, thanks for submitting. Appreciate your attention to detail with code conventions as well, makes it easier to read. Two minor comments. (1) in the equals method, shouldn't the line "if (params [i] != other.params [i] )" use the equals method instead? (2) Sorry to bug you on this, but we need you to resubmit "IntrospectionCacheDataTest.java" without the copyright notice. You still retain ownership of the code, but copyright notices need to go in a project-wide NOTICES file. (more info at: http://www.apache.org/legal/src-headers.html ) When you have a chance to resubmit, I'll commit.
        Hide
        Will Glass-Husain added a comment -

        Great, the download worked. Let me look at this and then commit unless I have questions. Thanks for submitting this (and thanks for the test case too, that helps).

        Show
        Will Glass-Husain added a comment - Great, the download worked. Let me look at this and then commit unless I have questions. Thanks for submitting this (and thanks for the test case too, that helps).
        Hide
        Will Glass-Husain added a comment -

        Thanks for contributing!

        for some reason the patch didn't upload properly - I get a Tomcat error when downloading.

        maybe it's a JIRA problem?

        can you upload again?

        Show
        Will Glass-Husain added a comment - Thanks for contributing! for some reason the patch didn't upload properly - I get a Tomcat error when downloading. maybe it's a JIRA problem? can you upload again?
        Hide
        Alexey Panchenko added a comment -

        ASTMethod_IntrospectionCacheData_cache-v2.patch is the updated patch

        Show
        Alexey Panchenko added a comment - ASTMethod_IntrospectionCacheData_cache-v2.patch is the updated patch

          People

          • Assignee:
            Will Glass-Husain
            Reporter:
            Alexey Panchenko
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development