Summary: | JSR223TestElement should cache ScriptEngineManager | ||
---|---|---|---|
Product: | JMeter - Now in Github | Reporter: | Philippe Mouawad <p.mouawad> |
Component: | Main | Assignee: | 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
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? 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 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. 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. 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.
Created attachment 29039 [details]
Test Plan
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. 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 This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/2843 |