Bug 53365

Summary: JSR223TestElement should cache ScriptEngineManager
Product: JMeter - Now in Github Reporter: Philippe Mouawad <p.mouawad>
Component: MainAssignee: JMeter issues mailing list <issues>
Status: RESOLVED FIXED    
Severity: enhancement CC: p.mouawad
Priority: P2    
Version: 2.7   
Target Milestone: ---   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 53520    
Attachments: Test Plan showing strange behaviour of memory
Path caching ScriptEngineManager
Test Plan

Description Philippe Mouawad 2012-06-05 21:16:01 UTC
Looking at code of JSR223TestElement, it seems to me we should:
- Cache ScriptEngineManager instead of creating it at each call which I think has a cost, see https://studio.plugins.atlassian.com/browse/GRV-6
- Remove initManager and introduce a similar method so that it puts variables in ScriptEngine instead of ScriptEngineManager so that we can reuse the latter

Waiting for your remarks to start this.

Regards
Philippe
Comment 2 Sebb 2012-06-08 16:39:44 UTC
This is probably OK, but need to ensure that the existing behaviour is not affected by using a shared Manager.

Also, is the Manager thread-safe?
Comment 3 Philippe Mouawad 2012-06-30 06:43:52 UTC
Created attachment 29012 [details]
Test Plan showing strange behaviour of memory

I made some tests with current attached test and noticed kind of big issue in JS233 approach.
With attached test plan, running it for some time and monitoring memory, I see a constant increase of memory, performance drop down although I don't get an OutOfMemoryError.
Memory after 2 minutes of run varies between 400 m and 500m (with -Xmx512m).

memory is occupied mainly by:
org.codehaus.groovy.reflection.ClassInfo$ClassInfoSet$Segment
com.sun.beans.AppContext

It may be a groovy memory leak or just how it works:
http://groovy.329449.n5.nabble.com/OOM-because-ScriptEnginge-Memory-caches-all-ever-compiled-Groovy-Classes-td387931.html
Comment 4 Philippe Mouawad 2012-06-30 06:55:06 UTC
JSR233 proposes Compilable interface and many ScriptEngine implement it

I think we should improve this part by doing something like that:
- We could compile script code
- then cache it (which key to use ? add a new property ? use Sampler Name ? use file script name ? use a hash of Script code ?)

I tested this approach:
- It fixes the memory leak issue
- It gives fabulous performance enhancements

But doing so means script code must not contains variables which would forbid caching the compiled code unless we hash script code to see if it changes but this will introduce a performance issue on this hashing.
Comment 5 Sebb 2012-06-30 08:58:13 UTC
The scripting elements were intended mainly for prototyping and ad-hoc testing; the intention being that the script would often be turned into a Java class if necessary for performance. So performance and memory consumption are not as important as for the Java classes.

That's not to say that improvements aren't useful, but such improvements must not affect backwards compatibility; in particular variables must be supported as before.

The JSR223 test elements support reading the script from a file, and parameters can be passed to the file, so one way to improve performance without affecting compatibility would be to cache the compiled form of a file using the key of file name+modification time.

The same could apply to the ScriptEngineManager: cache that if using a file.

This would guarantee compatibility, and probably not be too difficult to implement, nor be difficult for users to convert their test elements.
Comment 6 Philippe Mouawad 2012-07-07 18:39:39 UTC
Created attachment 29038 [details]
Path caching ScriptEngineManager

Here is the first step which does the following:
- It creates the ScripEngineManager as a Singleton
- It doesn't store in ScripEngineManager any variables anymore, it stores them in ScripEngine instead


Regarding backward compatibility:
- It removes getManager protected method and initManager
- It changes processFileOrScript signature
- It also changes scope of variables that were previously stored in ScripEngineManager

I made comparisons with attached Test Plan and it gives the following:
- JMeter 2.7 : 77 samples/sec
- ScriptEngineManager as instance variable of JSR223TestElement created once: 80 samples/sec
- This version 93.8 samples/sec


Regarding strange memory behaviour, it is strange but there is no leak, memory drops down although impacting performance in all cases.

In my understanding ScripEngineManager is thread safe.
Is it OK for you if I commit these changes ?


Regards
Philippe M.
Comment 7 Philippe Mouawad 2012-07-07 22:33:09 UTC
Created attachment 29039 [details]
Test Plan
Comment 8 Sebb 2012-07-08 12:45:16 UTC
I don't think the logging statement changes make sense.

Old: log.warn("Problem in JSR223 script "+e);
New: log.warn("Problem in JSR223 script "+e, e);

No point repeating "e" as text and stack trace.

The log should either be left alone or changed to:

log.warn("Problem in JSR223 script", e);

I don't think it's a problem changing the protected method API.

It looks as though the change to cache the ScriptEngineManager would be OK.
This does change the location of the variables, but AFAICT they still have the same life-time, i.e. just the sample.
Comment 9 Philippe Mouawad 2012-07-08 13:08:33 UTC
Date: Sun Jul  8 13:05:15 2012
New Revision: 1358733

URL: http://svn.apache.org/viewvc?rev=1358733&view=rev
Log:
Bug 53365 - JSR223TestElement should cache ScriptEngineManager
Bugzilla Id: 53365

Modified:
    jmeter/trunk/src/components/org/apache/jmeter/assertions/JSR223Assertion.java
    jmeter/trunk/src/components/org/apache/jmeter/extractor/JSR223PostProcessor.java
    jmeter/trunk/src/components/org/apache/jmeter/modifiers/JSR223PreProcessor.java
    jmeter/trunk/src/components/org/apache/jmeter/timers/JSR223Timer.java
    jmeter/trunk/src/components/org/apache/jmeter/visualizers/JSR223Listener.java
    jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java
    jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JSR223Sampler.java
    jmeter/trunk/xdocs/changes.xml
Comment 10 The ASF infrastructure team 2022-09-24 20:37:50 UTC
This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/2843