Bug 57114 - Performance : Functions that only have values as instance variable should not synchronize execute
Summary: Performance : Functions that only have values as instance variable should not...
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: Nightly (Please specify date)
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-18 12:28 UTC by Philippe Mouawad
Modified: 2014-10-27 13:44 UTC (History)
2 users (show)



Attachments
Avoid synchronization where it is not required (42.28 KB, patch)
2014-10-18 14:16 UTC, Vladimir Sitnikov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Mouawad 2014-10-18 12:28:20 UTC
As per Ubik Load Pack report:
-----------------------------------------------------------------------------
Reviewing code of Functions in JMeter we don't understand why Functions that only have values as instance variable (so Beanshell or StringFromFile or IterationCounter (which could be improved to avoid it) are not concerned) synchronize on execute.

It appears "values" instance variable is initialized within StandardJMeterEngine thread and then only accessed in read mode by JMeterThread

But it is true that if synchronized is removed then CompoundVariable is accessed by multi threads but it seems it uses thread related data so it should be OK.


-----------------------------------------------------------------------------

And as per dev mailing list :
http://mail-archives.apache.org/mod_mbox/jmeter-dev/201410.mbox/%3CCAB%3DJe-Gy4tp%3DMPih292oG1XpH7D1X0%2BUR%2BUa_rnLyXaUH-fePA%40mail.gmail.com%3E
Comment 1 Vladimir Sitnikov 2014-10-18 14:16:46 UTC
Created attachment 32124 [details]
Avoid synchronization where it is not required

I went through sub-classes of AbstractFunction and removed synchronization where it is not required.
Basically, if a function does not modify state and if it accesses thread-safe objects only, then synchronization is not required.

Please find the attached patch on top of e9228ccf / trunk@1632410.

Notes:
0) The patch contains just synchronization removal + some corrections to CV. All the tests pass except testPropfile(org.apache.jmeter.save.TestSaveService)junit.framework.AssertionFailedError: Property File Version mismatch, ensure you update SaveService#FILEVERSION field with revision id of saveservice.properties.

testPropfile fails without patch as well, so I did not investigate that.

1) I have no idea what to do with BeanShell function. bsh.Interpreter is thread safe, however bshInterpreter.set("vars",...) should not be accessed concurrently as that will just overwrite the value.
I think it makes sense to wrap bsh interpreter in a ThreadLocal, so synchronization can be removed from BeanShell.execute
Comment 2 Philippe Mouawad 2014-10-19 19:57:12 UTC
Date: Sun Oct 19 19:53:07 2014
New Revision: 1632983

URL: http://svn.apache.org/r1632983
Log:
Bug 57114 - Performance : Functions that only have values as instance variable should not synchronize execute
Bugzilla Id: 57114

Modified:
    jmeter/trunk/src/core/org/apache/jmeter/engine/util/CompoundVariable.java
    jmeter/trunk/src/core/org/apache/jmeter/functions/AbstractFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/AbstractHostIPName.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/CSVRead.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/CharFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/EscapeHtml.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/EscapeOroRegexpChars.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/EvalFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/EvalVarFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/FileToString.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/IntSum.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/IterationCounter.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/JavaScript.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/Jexl2Function.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/JexlFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/LogFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/LogFunction2.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/LongSum.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/Property.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/Property2.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/Random.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/RandomString.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/RegexFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/SamplerName.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/SetProperty.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/SplitFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/TestPlanName.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/TimeFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/UnEscape.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/UnEscapeHtml.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/UrlDecode.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/UrlEncode.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/Variable.java
    jmeter/trunk/xdocs/changes.xml
Comment 3 Philippe Mouawad 2014-10-27 13:44:22 UTC
Date: Mon Oct 27 13:41:47 2014
New Revision: 1634535

URL: http://svn.apache.org/r1634535
Log:
Bug 57114 - Performance : Functions that only have values as instance variable should not synchronize execute
Remove synchronized on setParameters
Bugzilla Id: 57114

Modified:
    jmeter/trunk/src/functions/org/apache/jmeter/functions/EscapeOroRegexpChars.java


Date: Mon Oct 27 13:43:01 2014
New Revision: 1634537

URL: http://svn.apache.org/r1634537
Log:
Bug 57114 - Performance : Functions that only have values as instance variable should not synchronize execute
Remove old TODO
Bugzilla Id: 57114

Modified:
    jmeter/trunk/src/functions/org/apache/jmeter/functions/SamplerName.java


Date: Mon Oct 27 13:42:36 2014
New Revision: 1634536

URL: http://svn.apache.org/r1634536
Log:
Bug 57114 - Performance : Functions that only have values as instance variable should not synchronize execute
Keep synchronized where it is needed
Bugzilla Id: 57114

Modified:
    jmeter/trunk/src/functions/org/apache/jmeter/functions/LogFunction.java
    jmeter/trunk/src/functions/org/apache/jmeter/functions/LogFunction2.java