• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 199
  • Last Modified:

Whats wrong with this code?

Hello,
I recently ran a profiler on my code because of the huge amounts of memory it began to utilize over time. It topped out at 850megs RAM, and 828 megs VM. According the profiler the only object that is getting ridiculously large is a char[] that is the product of a string.append / string.expandcapacity call caused by a function replaceAll that I wrote. For the life of me I cannot figure out why the garbage collector doesnt clean this up, but I watched it steadily increase as array after array after array were created. Here's the function in question and the code that calls it. The proper fix for this is worth 500 points.

Function:

String replaceAll(String toSearch, String Search, String value)
    {
          int start = -1;
          int end = -1;

          String workString = toSearch;

          while ((start = workString.indexOf(Search)) != -1)
          {
               end = workString.indexOf('@',start+1);
               workString = workString.substring(0,start) + value + workString.substring(end+1,workString.length());
          }

          return workString;
     }


Code:

  String message = content.replaceAll("@open@",open);
  message = replaceAll(message,"@to@",data.get("email").toString());
  message = replaceAll(message,"@href@",href);
  message = replaceAll(message,"@unsub@",unsub);
  message = replaceAll(message,"@from@",fromline);
  message = replaceAll(message,"@subject@",subject);
  message = replaceAll(message,"@msgId@",msgId);
  message = replaceAll(message,"@date@",date);
  message = replaceAll(message,"@advertiser_unsub@",aunsub);

Thanks,
Rick
0
richardsimnett
Asked:
richardsimnett
  • 6
  • 4
  • 3
  • +3
2 Solutions
 
gnoonCommented:
Rick,

The while loop in replaceAll() function will loops forever if the 'Search' argument is a substring of the 'value' argument,
and eats huge amounts of RAM.

Did you check it?
0
 
aozarovCommented:
I guess you are not using JDK 5.0.
Before 5.0 (and even a bit earlier, not sure when the change was applied) there were several bugs related to "memory leaks" or excessive memory usage when converting string / stringbuffer and calling substring. Most of them related to the fact that the orignal big string was copied/used also by the smaller substrings due the "shared" functionality (which was removed later on).
You can look at Java Bug parade to see those bugs: (4724129  -> Memory leak in use of StringBuffer.toString() , 4637640 -> Memory leak due to String.substring() implementation and others). If you dig hard you might find a solution to your problem but why not to use String.replaceAll (since 1.4) or jakarta ORO for JVM < 1.4. I am sure those will scale/perform better.
0
 
aozarovCommented:
Just to make it clear. Though you are not using StringBuffer directly those are being created when implementing Str1 + Str2.
This is where expandcapacity and append come from (they are not methods in the String class)
0
Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
TrigunaCommented:
Why you are going for your own code, as we know there is built-in replace method in String.

It will automatically replace the searchstring by search value.

Try to use the built-in methods.
0
 
CEHJCommented:
>>Why you are going for your own code, as we know there is built-in replace method in String.

Maybe richardsimnett is using < 1.4. If you aren't richardsimnett, you can use String.replaceAll, but be aware that the arguments are regular expressions, which may matter in certain cases
0
 
TrigunaCommented:
Oh!! Sorry.

I think I made a mistake.

Anyway thank you.
0
 
MogalManicCommented:
To answer you question!  You are searching for the ending '@' twice.  Once in the search string and again while computing the 'end' offset.  I reworked the function to compute 'end' correctly and used StringBuffer instead of String to use memory effeciently:
    String replaceAll(String toSearch, String Search, String value)
    {
      int start = -1;
      int end = -1;

      StringBuffer workString = new StringBuffer(toSearch);

      while ((start = workString.indexOf(Search)) != -1) {
            end = start+Search.length();
            workString.replace(start, end, value);
      }

      return workString.toString();
    }
0
 
richardsimnettAuthor Commented:
gnoon,
    That will never occur.

Thanks,
Rick
0
 
richardsimnettAuthor Commented:
aozarov,
Actually locally I am using jdk1.4.2, and on the production machine jdk1.5..... I am experiencing the same issue on both platforms. The reason for rewriting the replaceAll function as opposed to using the standard one was because our tokens were not properly replacing. So I rewrote it so that it didnt use regular expressions.

Thanks,
Rick
0
 
richardsimnettAuthor Commented:
CEHJ,
That is exactly the case here.

Thanks,
Rick
0
 
richardsimnettAuthor Commented:
MogalManic,
The code looks good. I just plugged it into my code, and compiled. I'll set it to run....  I should know tomorrow if this fixed the problem. It takes a long amount of execution time prior to the problem becoming apparent.

Thanks,
Rick


0
 
aozarovCommented:
BTW, arguments that can be regular expressions can be escaped by using org.apache.oro.text.regex.Perl5Compiler.quotemeta
Not sure why such functionality was not provided with 1.4 java.util.regex
0
 
richardsimnettAuthor Commented:
MogalManic,
Ok after some extensive run time it appears that your solution alleviated a portion of the problem. The memory leak is still there, however, its no longer in the same section of code. What would I replace statements like this with?

String t = t + <somevalue> + '"' + <somevalue>;

That is the only other thing within this class or program for this matter where these string leaks could be occurring.

Thanks,
Rick
0
 
gnoonCommented:
>What would I replace statements like this with?
>String t = t + <somevalue> + '"' + <somevalue>;

this should work

StringBuffer t = new StringBuffer();
t.append(<somevalue>).append("").append(<somevalue>);
0
 
aozarovCommented:
Why not to use stringBuffer only.
So if you take MogalManic aproach then do something like that:

void replaceAll(StringBuffer workString, String Search, String value)
    {
     int start = -1;
     int end = -1;

     while ((start = workString.indexOf(Search)) != -1) {
          end = start+Search.length();
          workString.replace(start, end, value);
     }
    }

Also, I think that can be optimized by calling indexOf(Search, end).
0
 
gnoonCommented:
>String message = content.replaceAll("@open@",open);
>message = replaceAll(message,"@to@",data.get("email").toString());
>message = replaceAll(message,"@href@",href);
>...

why you still using String.replaceAll().
why dont use <code>String message = replaceAll(content,"@open@",open);<code> instead.
0
 
richardsimnettAuthor Commented:
gnoon,
I caught that earlier, and changed it. THat is no longer how the code reads.

Thanks,
Rick
0

Featured Post

Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

  • 6
  • 4
  • 3
  • +3
Tackle projects and never again get stuck behind a technical roadblock.
Join Now