?
Solved

Whats wrong with this code?

Posted on 2005-03-22
17
Medium Priority
?
197 Views
Last Modified: 2010-03-31
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
Comment
Question by:richardsimnett
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 6
  • 4
  • 3
  • +3
17 Comments
 
LVL 16

Expert Comment

by:gnoon
ID: 13608048
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
 
LVL 15

Expert Comment

by:aozarov
ID: 13608091
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
 
LVL 15

Expert Comment

by:aozarov
ID: 13608116
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
Get 15 Days FREE Full-Featured Trial

Benefit from a mission critical IT monitoring with Monitis Premium or get it FREE for your entry level monitoring needs.
-Over 200,000 users
-More than 300,000 websites monitored
-Used in 197 countries
-Recommended by 98% of users

 
LVL 5

Expert Comment

by:Triguna
ID: 13608811
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
 
LVL 86

Expert Comment

by:CEHJ
ID: 13609918
>>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
 
LVL 5

Expert Comment

by:Triguna
ID: 13609926
Oh!! Sorry.

I think I made a mistake.

Anyway thank you.
0
 
LVL 21

Accepted Solution

by:
MogalManic earned 1600 total points
ID: 13610239
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
 

Author Comment

by:richardsimnett
ID: 13612383
gnoon,
    That will never occur.

Thanks,
Rick
0
 

Author Comment

by:richardsimnett
ID: 13612448
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
 

Author Comment

by:richardsimnett
ID: 13612479
CEHJ,
That is exactly the case here.

Thanks,
Rick
0
 

Author Comment

by:richardsimnett
ID: 13612545
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
 
LVL 15

Expert Comment

by:aozarov
ID: 13612666
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
 

Author Comment

by:richardsimnett
ID: 13618321
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
 
LVL 16

Assisted Solution

by:gnoon
gnoon earned 400 total points
ID: 13618865
>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
 
LVL 15

Expert Comment

by:aozarov
ID: 13618871
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
 
LVL 16

Expert Comment

by:gnoon
ID: 13618895
>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
 

Author Comment

by:richardsimnett
ID: 13619498
gnoon,
I caught that earlier, and changed it. THat is no longer how the code reads.

Thanks,
Rick
0

Featured Post

Free Tool: Port Scanner

Check which ports are open to the outside world. Helps make sure that your firewall rules are working as intended.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

By the end of 1980s, object oriented programming using languages like C++, Simula69 and ObjectPascal gained momentum. It looked like programmers finally found the perfect language. C++ successfully combined the object oriented principles of Simula w…
In this post we will learn how to connect and configure Android Device (Smartphone etc.) with Android Studio. After that we will run a simple Hello World Program.
Viewers learn about the “for” loop and how it works in Java. By comparing it to the while loop learned before, viewers can make the transition easily. You will learn about the formatting of the for loop as we write a program that prints even numbers…
This tutorial covers a practical example of lazy loading technique and early loading technique in a Singleton Design Pattern.
Suggested Courses
Course of the Month11 days, 11 hours left to enroll

752 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question