Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: lang.*
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: All

      Description

      The static field PADDING allocates 262152 bytes for nothing.

      private static final String[] PADDING = new String[Character.MAX_VALUE];

      At least use lazy creation, better completely remove it (move to method) to save
      memory (Having 20 apps on one tomcat --> 5MB allocated for nothing).

        Activity

        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        Fix commited. I choose to use a char[] array instead of a StringBuffer but
        otherwise it's just like what comment #3 describes.
        http://svn.apache.org/viewcvs?rev=384017&view=rev

        Show
        Sandy McArthur (from Bugzilla import) added a comment - Fix commited. I choose to use a char[] array instead of a StringBuffer but otherwise it's just like what comment #3 describes. http://svn.apache.org/viewcvs?rev=384017&view=rev
        Hide
        ggregory@seagullsw.com added a comment -

        I think we should let call sites decide whether or not Strings should be interned.

        Show
        ggregory@seagullsw.com added a comment - I think we should let call sites decide whether or not Strings should be interned.
        Hide
        sebastian.kirsch added a comment -

        (In reply to comment #3)
        I agree to this proposal, would be my preferred solution.

        Show
        sebastian.kirsch added a comment - (In reply to comment #3) I agree to this proposal, would be my preferred solution.
        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        One more thing, if it is desirable to keep the number of equal string instances
        low then the return could be changed to:

        return sb.toString().intern();

        But if Java 1.1 is a target for lang then that shouldn't be used as it will leak
        memory. It wasn't until Java 1.2 that the intered string pool would allow unused
        intered strings to be garbage collected.

        Show
        Sandy McArthur (from Bugzilla import) added a comment - One more thing, if it is desirable to keep the number of equal string instances low then the return could be changed to: return sb.toString().intern(); But if Java 1.1 is a target for lang then that shouldn't be used as it will leak memory. It wasn't until Java 1.2 that the intered string pool would allow unused intered strings to be garbage collected.
        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        Has it been shown that the PADDING caching actually benefits anything? I think
        it should be removed as it's just going to consume more and more memory until
        the StringUtils class is unloaded.

        Either way the StringUtils.padding(int,char) should be changed to use a
        StringBuffer. I'd change the method to:

        private static String padding(int repeat, char padChar) {
        StringBuffer sb = new StringBuffer(repeat);
        while (sb.length() < repeat)

        { pad = pad.concat(pad); }

        return sb.toString();
        }

        and remove the PADDING static field.

        Show
        Sandy McArthur (from Bugzilla import) added a comment - Has it been shown that the PADDING caching actually benefits anything? I think it should be removed as it's just going to consume more and more memory until the StringUtils class is unloaded. Either way the StringUtils.padding(int,char) should be changed to use a StringBuffer. I'd change the method to: private static String padding(int repeat, char padChar) { StringBuffer sb = new StringBuffer(repeat); while (sb.length() < repeat) { pad = pad.concat(pad); } return sb.toString(); } and remove the PADDING static field.
        Hide
        ggregory@seagullsw.com added a comment -

        Seems like a good idea. Since PADDING is private, the change will not create an
        incompatibility.

        Show
        ggregory@seagullsw.com added a comment - Seems like a good idea. Since PADDING is private, the change will not create an incompatibility.
        Hide
        Henri Yandell added a comment -

        I favour switching to lazy loading; have asked on the mailng list how others feel.

        Show
        Henri Yandell added a comment - I favour switching to lazy loading; have asked on the mailng list how others feel.

          People

          • Assignee:
            Unassigned
            Reporter:
            sebastian.kirsch
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development