Bug 40850 - - Memory leak in all BeanShell components
- Memory leak in all BeanShell components
Status: RESOLVED FIXED
Product: JMeter
Classification: Unclassified
Component: Main
2.2
PC Windows XP
: P2 major (vote)
: ---
Assigned To: JMeter issues mailing list
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2006-10-31 02:58 UTC by Nikolay Diakov
Modified: 2010-01-28 16:06 UTC (History)
1 user (show)



Attachments
example of patching a component that uses BeanShell (3.69 KB, text/plain)
2006-11-02 03:49 UTC, Nikolay Diakov
Details
- the interpreter patched (8.45 KB, text/plain)
2006-11-02 03:51 UTC, Nikolay Diakov
Details
Patch to resolve memory leak issue (17.18 KB, text/plain)
2006-12-18 12:08 UTC, Eric Dalquist
Details
Add option to reset bsh.Interpreter on each iteration (19.28 KB, patch)
2008-04-04 09:47 UTC, Eric Dalquist
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolay Diakov 2006-10-31 02:58:25 UTC
We heavily use BeanShell pre-, post- processor and function components.

We discovered that after running for 10 minutes, we get OutOfMemoryExceptions, 
consistenly.

We made some test with simplified scripts and we have te same - even simple 
BeanShell scripts cause leaks.

It turns out that this problem actually originates in the BeanShell interpreter 
itself. The only workaround I have found constitutes of throwing out the whole 
interpreter object afer an eval.

Looking at the sources, at the moment JMeter uses one isntance per component, 
per test run. 

To see whether the workaround can work for JMeter, we have patched the 
BeanShell components to instantiate a interpreters any time they need to 
evaluate a script. This helped and now we have bounded memory use. We probably 
have a performance loss, but so far this has posed no problem.

Since I do not see BeanShell devs fixing their leak, perhaps JMeter should 
integrate BeanShell using the workaround, to keep JMeter actually usable with 
this scripting?

Cheers,
  Nik
Comment 1 Guillaume Lasnier 2006-11-02 01:37:14 UTC
Could you attach the patch?

(In reply to comment #0)
> We heavily use BeanShell pre-, post- processor and function components.
> 
> We discovered that after running for 10 minutes, we get OutOfMemoryExceptions, 
> consistenly.
> 
> We made some test with simplified scripts and we have te same - even simple 
> BeanShell scripts cause leaks.
> 
> It turns out that this problem actually originates in the BeanShell interpreter 
> itself. The only workaround I have found constitutes of throwing out the whole 
> interpreter object afer an eval.
> 
> Looking at the sources, at the moment JMeter uses one isntance per component, 
> per test run. 
> 
> To see whether the workaround can work for JMeter, we have patched the 
> BeanShell components to instantiate a interpreters any time they need to 
> evaluate a script. This helped and now we have bounded memory use. We probably 
> have a performance loss, but so far this has posed no problem.
> 
> Since I do not see BeanShell devs fixing their leak, perhaps JMeter should 
> integrate BeanShell using the workaround, to keep JMeter actually usable with 
> this scripting?
> 
> Cheers,
>   Nik

Comment 2 Nikolay Diakov 2006-11-02 03:49:39 UTC
Created attachment 19070 [details]
example of patching a component that uses BeanShell

This file exemplifies how I patched some of the components. I did not patch all
of them, only pre-, post-, and function.

I also changed the BeanShellInterpreter.java (see separate attachment). Perhaps
one can contain all necessary changes to that file - I did not have time to do
this.
Comment 3 Nikolay Diakov 2006-11-02 03:51:10 UTC
Created attachment 19071 [details]
- the interpreter patched

Clearing the name space seems required as BeanShell has some static structure
in there.
Comment 4 Eric Dalquist 2006-12-18 12:08:27 UTC
Created attachment 19281 [details]
Patch to resolve memory leak issue

Minimal diff to fix the BeanShell memory leak. The patch changes the
BeanShellInterpreter to create an instance of the bsh.Interpreter class each
time eval or source is called. Variables created in the Interpreter are loaded
into a Map after each evaluation and loaded back into the new Interpreter
before each evaluation to keep the same state between runs.
Comment 5 Sebb 2007-03-09 12:54:58 UTC
I've just done an experiment with a very simple script:

Thread.sleep(200);
return "1";

When this is run from the script pane, memory usage does climb as the test 
continues.

However, when the script is executed from a file, memory usage sees to 
increase very slowly.

Perhaps you can try this as a work-round.
Comment 6 Eric Dalquist 2008-04-02 11:10:12 UTC
I'm in the process of upgrading to jMeter 2.3.1 from 2.2 and re-tested our script on a vanilla 2.3.1 instance to see if the memory issue still exists. It does and in 684 samples from a single thread the BSH Listener was close to causing an out-of-memory error.

I was monitoring the script execution using JConsole and have screen shots of the overall heap usage and old-gen usage after stopping the test. I also used jmap to get historgrams of the live object count/size immediately before and after stopping the test.

These files along with the jmx script and bsh script are available here: https://mywebspace.wisc.edu/dalquist/web/jMeterMemoryLeak/

Note that the configuration files that specify the hosts and users used in the test are not included so the script is not runnable as is.


We'll be doing a vendor branch import of 2.3.1 later this week and migrating our BSH component fix, which is attached to this issue as "Patch to resolve memory leak issue", to the 2.3.1 code and I'll post an updated patch if needed.
Comment 7 Sebb 2008-04-03 04:53:40 UTC
I would be interested in seeing a patch against 2.3.1.

I've just had another look at the 2006-12-18 patch, but it does not seem to be possible to apply it to BeanShellInterpreter, even if I revert the source to revision 488388. Most of the patch ends up being rejected.

Examining the patch manually is also really difficult, as there are a lot of renames and method signature changes.

If you do provide another patch, it would be very helpful if it could be just the minimum needed.

I can then look at incorporating it into the next release.

In the meantime, could you perhaps attach the full source for your copy of BeanShellInterpreter.java? I might be able to make some headway with that.
Comment 8 Eric Dalquist 2008-04-03 13:38:15 UTC
I'm working on applying the patch to 2.3.1 now and should have a patch by the end of the week.

The essence of what the previous patch did was move the bshClass.newInstance(); call from the BeanShellInterpreter constructor to the bshInvoke method so a new interpreter instance is created each time to release the handle on the previously parsed script objects.

The downside of this approach is the extra object creation and the inability to pass data between BSH runs other than through jMeter properties. As far as I can tell from the BSH docs there is no way to give a bsh.Interpreter a script and just a have it run that same script over and over.

I was thinking that this time around I could look at adding an option to the UI to either use a persistent Interpreter (realizing it leaks memory) or a per-call Interpreter which won't leak memory.
Comment 9 Sebb 2008-04-03 13:57:05 UTC
(In reply to comment #8)
> I'm working on applying the patch to 2.3.1 now and should have a patch by the
> end of the week.
 
Thanks.

> The essence of what the previous patch did was move the bshClass.newInstance();
> call from the BeanShellInterpreter constructor to the bshInvoke method so a new
> interpreter instance is created each time to release the handle on the
> previously parsed script objects.
> 
> The downside of this approach is the extra object creation and the inability to pass data between BSH runs other than through jMeter properties. 

Yes; that's why I've been wary of adding it.

> As far as I
> can tell from the BSH docs there is no way to give a bsh.Interpreter a script
> and just a have it run that same script over and over.

You can define methods in a startup file, and call that from the screen or another file. This reduces the new objects created.
 
> I was thinking that this time around I could look at adding an option to the UI
> to either use a persistent Interpreter (realizing it leaks memory) or a
> per-call Interpreter which won't leak memory.
> 

It could be UI or a property.
Comment 10 Eric Dalquist 2008-04-04 09:47:46 UTC
Created attachment 21783 [details]
Add option to reset bsh.Interpreter on each iteration

This patch is a slightly different approach to the problem than the patch for 2.2. 

In BeanShellInterpreter the code that creates the new bsh.Interpreter instance and that calls init(String, Logger) has been moved into a public reset() method. This method is called by both constructors to retain the existing behavior but gives clients of the class a way to re-created and re-init the Interpreter.

All of the jMeter components that use the BeanShellInterpreter have been updated to provide a per-component configuration option to reset the interpreter on each iteration, this defaults to false.

All of the BeanShellBeanInfoSupport based UIs use a select box for the true/false selection, the assertion and sampler GUIs use a checkbox. It seems like a checkbox would be preferable for the BeanShellBeanInfoSupport based UIs but I couldn't figure out an easy way to add a JCheckBox to them.

I did not update the BeanShell function as I'm not sure if that needs the option as well, if you think it does let me know and I can add an option for a reset call there as well.

The patch should be the absolute minimal diff based on 2.3.1, please let me know if you have problems applying it or questions about the patch.

Thank you,
Eric
Comment 11 Sebb 2008-04-04 12:24:09 UTC
Thanks very much. The patch worked OK with the current code (after 2.3.1) with only a minor tweak.

I had to change it slightly, because public functions cannot be called from constructors. In the end, I merged the init() and reset() methods, as there seemed no reason to keep them separate.

I've committed the changes in:
http://svn.apache.org/viewvc?view=rev&revision=644823
and
http://svn.apache.org/viewvc?rev=644831&view=rev
If you want to test them, they are in the jakarta-jmeter-r644833 nightly build (and later ones).

A previous patch also saved and restored variables.
Would this still be useful, perhaps as a 3rd option?
I.e reuse / reset / reset,keeping variables

I don't think the BeanShell function needs a reset option.

[As to TestBean checkboxes - they have not been implemented.]
Comment 12 Eric Dalquist 2008-04-04 12:48:55 UTC
Thanks for applying the patch, combining init and reset is a good move I just wanted to keep the diffs as small as possible.

As for copying the data between runs, it shouldn't be too hard logically bug doing it all via reflection may be painful as there are a few other BSH objects that look like they need to be referenced to make it happen. If it is an important feature let me know and I can see about implementing it.
Comment 13 Sebb 2008-05-08 04:33:50 UTC
If BeanShell scripts need to save data between invocations, as a work-round, they can use JMeter variables.

Closing the bug.

If some form of automatic variable saving is needed, that would be best dealt with in an enhancement request.
Comment 14 Gabe 2010-01-28 15:42:04 UTC
What about using Beanshell as a function, via __BeanShell?

I'm seeing a massive drop in performance in my test plan after 5-10minutes, and I use beanshell functions inside my sampler (typically via a source() call). I believe the root cause is the same. From inspecting the code and the patch it doesn't appear that there is a way to reset the interpreter on each function call.
Comment 15 Sebb 2010-01-28 16:06:17 UTC
Using beanshell.function.init instead of using source() should reduce memory usage. You can define various functions in the same file.

If you wish to add a reset option to the __BeanShell function, please open a new Buzilla issue.