You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by sebb <se...@gmail.com> on 2016/01/26 11:21:50 UTC

Re: svn commit: r1726684 - in /jmeter/trunk: src/core/org/apache/jmeter/util/JSR223TestElement.java src/core/org/apache/jmeter/util/ScriptingBeanInfoSupport.java xdocs/changes.xml

On 25 January 2016 at 20:44,  <pm...@apache.org> wrote:
> Author: pmouawad
> Date: Mon Jan 25 20:44:55 2016
> New Revision: 1726684
>
> URL: http://svn.apache.org/viewvc?rev=1726684&view=rev
> Log:
> Bug 56554 : the script cache key is now automatically generated
> #resolve #83
> Bugzilla Id: 56554
>
> Modified:
>     jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java
>     jmeter/trunk/src/core/org/apache/jmeter/util/ScriptingBeanInfoSupport.java
>     jmeter/trunk/xdocs/changes.xml
>
> Modified: jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java?rev=1726684&r1=1726683&r2=1726684&view=diff
> ==============================================================================
> --- jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java (original)
> +++ jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java Mon Jan 25 20:44:55 2016
> @@ -34,6 +34,7 @@ import javax.script.ScriptEngine;
>  import javax.script.ScriptEngineManager;
>  import javax.script.ScriptException;
>
> +import org.apache.commons.codec.digest.DigestUtils;
>  import org.apache.commons.collections.map.LRUMap;
>  import org.apache.commons.io.IOUtils;
>  import org.apache.commons.lang3.StringUtils;
> @@ -66,8 +67,12 @@ public abstract class JSR223TestElement
>
>      private static final long serialVersionUID = 233L;
>
> -    private String cacheKey = ""; // If not empty then script in ScriptText will be compiled and cached
> -
> +    /** If not empty then script in ScriptText will be compiled and cached */
> +    private String cacheKey = "";
> +
> +    /** md5 of the script, used as an unique key for the cache */
> +    private String scriptMd5 = null;
> +

-1

I already mentioned this on the Bugzilla - the md5 of the script may
change, so needs to be calculated each time, and NOT cached.

>      /**
>       * Cache of compiled scripts
>       */
> @@ -182,21 +187,20 @@ public abstract class JSR223TestElement
>              }  else {
>                  throw new ScriptException("Script file '"+scriptFile.getAbsolutePath()+"' does not exist or is unreadable for element:"+getName());
>              }
> -        } else if (!StringUtils.isEmpty(getScript())){
> +        } else if (!StringUtils.isEmpty(getScript())) {
>              if (supportsCompilable && !StringUtils.isEmpty(cacheKey)) {
> -                CompiledScript compiledScript =
> -                        compiledScriptsCache.get(cacheKey);
> -                if (compiledScript==null) {
> +                computeScriptMD5();
> +                CompiledScript compiledScript = compiledScriptsCache.get(this.scriptMd5);
> +                if (compiledScript == null) {
>                      synchronized (compiledScriptsCache) {
> -                        compiledScript =
> -                                compiledScriptsCache.get(cacheKey);
> -                        if (compiledScript==null) {
> -                            compiledScript =
> -                                    ((Compilable) scriptEngine).compile(getScript());
> -                            compiledScriptsCache.put(cacheKey, compiledScript);
> +                        compiledScript = compiledScriptsCache.get(this.scriptMd5);
> +                        if (compiledScript == null) {
> +                            compiledScript = ((Compilable) scriptEngine).compile(getScript());
> +                            compiledScriptsCache.put(this.scriptMd5, compiledScript);
>                          }
>                      }
>                  }
> +
>                  return compiledScript.eval(bindings);
>              } else {
>                  return scriptEngine.eval(getScript(), bindings);
> @@ -206,6 +210,15 @@ public abstract class JSR223TestElement
>          }
>      }
>
> +    /**
> +     * compute MD5 if it is null
> +     */
> +    private void computeScriptMD5() {
> +        // compute the md5 of the script if needed
> +        if(scriptMd5 == null) {

-1

The hash MUST be calculated each time.

> +            scriptMd5 = DigestUtils.md5Hex(getScript());
> +        }
> +    }
>
>      /**
>       * @return the cacheKey
> @@ -251,7 +264,9 @@ public abstract class JSR223TestElement
>      @Override
>      public void testEnded(String host) {
>          compiledScriptsCache.clear();
> +        this.scriptMd5 = null;
>      }
> +
>      public String getScriptLanguage() {
>          return scriptLanguage;
>      }
>

Re: svn commit: r1726684 - in /jmeter/trunk: src/core/org/apache/jmeter/util/JSR223TestElement.java src/core/org/apache/jmeter/util/ScriptingBeanInfoSupport.java xdocs/changes.xml

Posted by Philippe Mouawad <ph...@gmail.com>.
On Tuesday, January 26, 2016, sebb <se...@gmail.com> wrote:

> On 25 January 2016 at 20:44,  <pmouawad@apache.org <javascript:;>> wrote:
> > Author: pmouawad
> > Date: Mon Jan 25 20:44:55 2016
> > New Revision: 1726684
> >
> > URL: http://svn.apache.org/viewvc?rev=1726684&view=rev
> > Log:
> > Bug 56554 : the script cache key is now automatically generated
> > #resolve #83
> > Bugzilla Id: 56554
> >
> > Modified:
> >     jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java
> >
>  jmeter/trunk/src/core/org/apache/jmeter/util/ScriptingBeanInfoSupport.java
> >     jmeter/trunk/xdocs/changes.xml
> >
> > Modified:
> jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java?rev=1726684&r1=1726683&r2=1726684&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java
> (original)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/util/JSR223TestElement.java
> Mon Jan 25 20:44:55 2016
> > @@ -34,6 +34,7 @@ import javax.script.ScriptEngine;
> >  import javax.script.ScriptEngineManager;
> >  import javax.script.ScriptException;
> >
> > +import org.apache.commons.codec.digest.DigestUtils;
> >  import org.apache.commons.collections.map.LRUMap;
> >  import org.apache.commons.io.IOUtils;
> >  import org.apache.commons.lang3.StringUtils;
> > @@ -66,8 +67,12 @@ public abstract class JSR223TestElement
> >
> >      private static final long serialVersionUID = 233L;
> >
> > -    private String cacheKey = ""; // If not empty then script in
> ScriptText will be compiled and cached
> > -
> > +    /** If not empty then script in ScriptText will be compiled and
> cached */
> > +    private String cacheKey = "";
> > +
> > +    /** md5 of the script, used as an unique key for the cache */
> > +    private String scriptMd5 = null;
> > +
>
> -1
>
> I already mentioned this on the Bugzilla - the md5 of the script may
> change, so needs to be calculated each time, and NOT cached.


before this commit, the compiled script used the code after fiest variable
replacement, no recompilation would occur even if variables changed.

So behaviour is not changed.

Computing md5 each time is overkill knowing that variables are better used
by passing them as parameters.



>
> >      /**
> >       * Cache of compiled scripts
> >       */
> > @@ -182,21 +187,20 @@ public abstract class JSR223TestElement
> >              }  else {
> >                  throw new ScriptException("Script file
> '"+scriptFile.getAbsolutePath()+"' does not exist or is unreadable for
> element:"+getName());
> >              }
> > -        } else if (!StringUtils.isEmpty(getScript())){
> > +        } else if (!StringUtils.isEmpty(getScript())) {
> >              if (supportsCompilable && !StringUtils.isEmpty(cacheKey)) {
> > -                CompiledScript compiledScript =
> > -                        compiledScriptsCache.get(cacheKey);
> > -                if (compiledScript==null) {
> > +                computeScriptMD5();
> > +                CompiledScript compiledScript =
> compiledScriptsCache.get(this.scriptMd5);
> > +                if (compiledScript == null) {
> >                      synchronized (compiledScriptsCache) {
> > -                        compiledScript =
> > -                                compiledScriptsCache.get(cacheKey);
> > -                        if (compiledScript==null) {
> > -                            compiledScript =
> > -                                    ((Compilable)
> scriptEngine).compile(getScript());
> > -                            compiledScriptsCache.put(cacheKey,
> compiledScript);
> > +                        compiledScript =
> compiledScriptsCache.get(this.scriptMd5);
> > +                        if (compiledScript == null) {
> > +                            compiledScript = ((Compilable)
> scriptEngine).compile(getScript());
> > +                            compiledScriptsCache.put(this.scriptMd5,
> compiledScript);
> >                          }
> >                      }
> >                  }
> > +
> >                  return compiledScript.eval(bindings);
> >              } else {
> >                  return scriptEngine.eval(getScript(), bindings);
> > @@ -206,6 +210,15 @@ public abstract class JSR223TestElement
> >          }
> >      }
> >
> > +    /**
> > +     * compute MD5 if it is null
> > +     */
> > +    private void computeScriptMD5() {
> > +        // compute the md5 of the script if needed
> > +        if(scriptMd5 == null) {
>
> -1
>
> The hash MUST be calculated each time.


As per my above note, -1 for this idea.
There is no regression introduced.
User who wants variables can uncheck the cache checkbox.




>
> > +            scriptMd5 = DigestUtils.md5Hex(getScript());
> > +        }
> > +    }
> >
> >      /**
> >       * @return the cacheKey
> > @@ -251,7 +264,9 @@ public abstract class JSR223TestElement
> >      @Override
> >      public void testEnded(String host) {
> >          compiledScriptsCache.clear();
> > +        this.scriptMd5 = null;
> >      }
> > +
> >      public String getScriptLanguage() {
> >          return scriptLanguage;
> >      }
> >
>


-- 
Cordialement.
Philippe Mouawad.