PR https://github.com/apache/jmeter/pull/67

The script cache key is now automatically generated

The md5 of the script content is used.
The previous cacheKey property is still there for compatibility but can
not be modified or viewed in the gui.
Note that with this change, a Compilable script will always be cached,
it was not the case with the previous version if the user set the
cacheKey property to an empty value.
Documentation was not updated.

(In reply to benoit.wiart from comment #1)
> PR https://github.com/apache/jmeter/pull/67
> 
> The script cache key is now automatically generated

The code caches the hash of the script.
However the script is not guaranteed constant, as there may be variable references in it. And for files, the file may change.
 
> The md5 of the script content is used.
> The previous cacheKey property is still there for compatibility but can
> not be modified or viewed in the gui.
> Note that with this change, a Compilable script will always be cached,
> it was not the case with the previous version if the user set the
> cacheKey property to an empty value.

I don't agree with this change, better to use a compile flag as per the original bug description.

> Documentation was not updated.

Hello Sebb,

I don't understand your remarks


>>And for files, the file may change.
How my patch change the current behaviour ?
the file script does not use the cache key in gui.
In jmeter 2.13 for file script the cache key is the the file path + language + last modification date.

Note that in jmeter 2.13 :
- there is no variable replacement in the script file
- the script files are always cached


>>I don't agree with this change
Which one ?
the key automatically generated OR the Compilable script always cached

(In reply to benoit.wiart from comment #3)
> Hello Sebb,
> 
> I don't understand your remarks
> 

Which remarks?
 
> >>And for files, the file may change.
> How my patch change the current behaviour ?
> the file script does not use the cache key in gui.
> In jmeter 2.13 for file script the cache key is the the file path + language
> + last modification date.

OK, then it's not an issue for files.
 
> Note that in jmeter 2.13 :
> - there is no variable replacement in the script file

However GUI script boxes may have variables so the hash must be calculated each time, rather than cached in an instance variable.

> - the script files are always cached
> 
> 
> >>I don't agree with this change
> Which one ?
> the key automatically generated OR the Compilable script always cached

"Compilable script always cached".

This is a behavioural change.

It would be better to replace the cache key field with a checkbox.
If the variable was set in the test plan, then select the checkbox.

Rather than create a new property, the code can treat empty/missing value as false and anything else as true. This will mean JMX files won't change contents unnecessarily.

(In reply to Sebb from comment #4)
> (In reply to benoit.wiart from comment #3)
...
> > >>I don't agree with this change
> > Which one ?
> > the key automatically generated OR the Compilable script always cached
> 
> "Compilable script always cached".
> 
> This is a behavioural change.

Plus there might be implementations, where one does not want the script to be cached/compiled.

@Felix

In jmeter 2.13, if the script engine support Compilable then the script will always be compiled.
Plus the logic is not the same for the scripts configured from file as they will be always cached (no need to set a cachekey in the gui)

I agree that we should maintain a backward compatibility for script with variables, but, in a future version, variables should not be permitted in the script : you should use the bindings and / or the script parameters.

The current code (cache key) is needed for the performance but it's really error prone :
- it does not work the same way for script and script file
- if you copy paste a jsr element and you forgot to change the cache key it will execute the script you have copied
- it does not give the best performance out-of-the-box as users can forgot to set the cache key

(In reply to benoit.wiart from comment #6)
> @Felix
> 
> In jmeter 2.13, if the script engine support Compilable then the script will
> always be compiled.

However caching is a separate issue.

> Plus the logic is not the same for the scripts configured from file as they
> will be always cached (no need to set a cachekey in the gui)

> I agree that we should maintain a backward compatibility for script with
> variables, but, in a future version, variables should not be permitted in
> the script : you should use the bindings and / or the script parameters.

Disagree - there's absolutely no reason to disallow script variables.

Even though bindings/parameters are more efficient, we should not disable the behaviour, not least because it will break some test plans.
 
> The current code (cache key) is needed for the performance but it's really
> error prone :
> - it does not work the same way for script and script file
> - if you copy paste a jsr element and you forgot to change the cache key it
> will execute the script you have copied
> - it does not give the best performance out-of-the-box as users can forgot
> to set the cache key

However for the careful user it does give full control over caching.

Indeed maybe we should allow the user to diable script file caching if they wish.

(In reply to benoit.wiart from comment #6)
> The current code (cache key) is needed for the performance but it's really
> error prone :
> - it does not work the same way for script and script file
> - if you copy paste a jsr element and you forgot to change the cache key it
> will execute the script you have copied
> - it does not give the best performance out-of-the-box as users can forgot
> to set the cache key

Completely agree. #2 is especially poor.

> we should maintain a backward compatibility for script with
> variables, but, in a future version, variables should not be permitted in
> the script : you should use the bindings and / or the script parameters.

This is questonable, no obvious reason to disallow them. But also there is no need for user to manage cache keys. The only thing important for user is caching itself (thus the possible option is "Cache" checkbox). It should be clarified precisely in documentation that if caching is chosen (wich should be available only if scripting engine is able to do this) then once substituted variables won't ever be changed.

(In reply to Sebb from comment #7)
> However for the careful user it does give full control over caching.

Could you please provide a real use-case when user really gains any real profit from managing cache keys?

> Indeed maybe we should allow the user to diable script file caching if they
> wish.

What could be reason for this "wish"? I can imagine only on-the-fly code generation to file but for this case Script Text field is enough.

(In reply to Vladimir Ryabtsev from comment #8)
> (In reply to benoit.wiart from comment #6)
... 
> > we should maintain a backward compatibility for script with
> > variables, but, in a future version, variables should not be permitted in
> > the script : you should use the bindings and / or the script parameters.
> 
> This is questonable, no obvious reason to disallow them. But also there is
> no need for user to manage cache keys. The only thing important for user is
> caching itself (thus the possible option is "Cache" checkbox). 

Agreed.

> It should be
> clarified precisely in documentation that if caching is chosen (wich should
> be available only if scripting engine is able to do this) then once
> substituted variables won't ever be changed.

The test element code does not see the raw script at runtime; it only sees the text after any variable references have been replaced. Variable replacement is managed by core JMeter code.

However if the variables are frequently changed, the cache may start to fill up with cache entries that may never be re-used. This is inefficient, so the user should be warned not to cache such scripts. However I don't think we should prevent the user from using variable references.

(In reply to Vladimir Ryabtsev from comment #9)
> (In reply to Sebb from comment #7)
> > However for the careful user it does give full control over caching.
> 
> Could you please provide a real use-case when user really gains any real
> profit from managing cache keys?

There is no need to allow the user to manage cache keys.
However we should allow users to control whether or not the script is cached.

> > Indeed maybe we should allow the user to diable script file caching if they
> > wish.
> 
> What could be reason for this "wish"? I can imagine only on-the-fly code
> generation to file but for this case Script Text field is enough.

Consistency with the GUI case.
Also for testing how much effect caching has on performance.
There may be apps where it is tricky to provide caching; this would help show whether or not adding caching would help.
There are possibly other usage cases.

On the fly code generation is catered for by checking the file mod time.

(In reply to Sebb from comment #10)
> if the variables are frequently changed, the cache may start to fill
> up with cache entries that may never be re-used. This is inefficient, so the
> user should be warned not to cache such scripts. However I don't think we
> should prevent the user from using variable references.

+1
"Cache" checkbox and strong advice (directly in GUI) not to use caching if Script Text gets lot of unique values of variables.

Concerning backward compatibility. Trivaial solution is:
- cache key is empty -> checkbox is cleared,
- cache key is set -> checkbox is checked.
But in the first case we may fall into issue mentioned by you: if variables are getting new values each time cache is growing. So, may be like this?
- cache key is empty -> checkbox is cleared,
- cache key is set -> checkbox is in intermediate state (in this case test element works same way as in v. 2.13),
- when checkbox gets switched from intermediate state to any explicit state the cache key is removed from jmx.

(In reply to benoit.wiart from comment #6)
> @Felix
> 
> In jmeter 2.13, if the script engine support Compilable then the script will
> always be compiled.
> Plus the logic is not the same for the scripts configured from file as they
> will be always cached (no need to set a cachekey in the gui)

This is only true, if the source of the script is a file. If it is not a file, than it will only get compiled, if a cache key is given.

> 
> I agree that we should maintain a backward compatibility for script with
> variables, but, in a future version, variables should not be permitted in
> the script : you should use the bindings and / or the script parameters.
> 
> The current code (cache key) is needed for the performance but it's really
> error prone :
> - it does not work the same way for script and script file
> - if you copy paste a jsr element and you forgot to change the cache key it
> will execute the script you have copied
> - it does not give the best performance out-of-the-box as users can forgot
> to set the cache key

I think it is nice, if we could replace the manual key with something automated, but I fear, that there are use cases, where disabling caching for individual scripts is needed.

(In reply to Felix Schumacher from comment #13)
> (In reply to benoit.wiart from comment #6)
> > @Felix
> > 
> > In jmeter 2.13, if the script engine support Compilable then the script will
> > always be compiled.
> > Plus the logic is not the same for the scripts configured from file as they
> > will be always cached (no need to set a cachekey in the gui)
> 
> This is only true, if the source of the script is a file. If it is not a
> file, than it will only get compiled, if a cache key is given.
> 
> > 
> > I agree that we should maintain a backward compatibility for script with
> > variables, but, in a future version, variables should not be permitted in
> > the script : you should use the bindings and / or the script parameters.
> > 
> > The current code (cache key) is needed for the performance but it's really
> > error prone :
> > - it does not work the same way for script and script file
> > - if you copy paste a jsr element and you forgot to change the cache key it
> > will execute the script you have copied
> > - it does not give the best performance out-of-the-box as users can forgot
> > to set the cache key
> 
> I think it is nice, if we could replace the manual key with something
> automated, but I fear, that there are use cases, where disabling caching for
> individual scripts is needed.

If we have the checkbox, don't we answer this use case ?
I think  Vladimir Ryabtsev  solution looks ok no ? Modulo the fact that checkbox must be disabled if language is not compilable

I rather follow Benoit on the variables not being permitted, for example with Groovy it is broken as this is a Groovy syntax (http://groovy-lang.org/syntax.html), and the good way to do this for any language, is to pass the variable in parameters, no ?
We can keep it but as Vladimir proposes add a warning.


Caching IS the correct way for languages providing Compilation, in some dev discussion, I gave some numbers on throughput impact for Groovy for example with or without Compilation.

(In reply to Philippe Mouawad from comment #14)
> 
> I rather follow Benoit on the variables not being permitted, for example
> with Groovy it is broken as this is a Groovy syntax
> (http://groovy-lang.org/syntax.html), and the good way to do this for any
> language, is to pass the variable in parameters, no ?

The fact that Groovy uses the same syntax is a nuisance, but is no reason to ban variables for every script language.

> We can keep it but as Vladimir proposes add a warning.

We MUST keep it because dropping it would break test plans that rely on it.

The simplest solution would be to ban Groovy GUI scripts and only allow Groovy files.

Remember that variable processing is not under the control of the test elements; it is done before the test element sees the script.

(In reply to Sebb from comment #15)
> (In reply to Philippe Mouawad from comment #14)
> > 
> > I rather follow Benoit on the variables not being permitted, for example
> > with Groovy it is broken as this is a Groovy syntax
> > (http://groovy-lang.org/syntax.html), and the good way to do this for any
> > language, is to pass the variable in parameters, no ?
> 
> The fact that Groovy uses the same syntax is a nuisance, but is no reason to
> ban variables for every script language.
ok
> 
> > We can keep it but as Vladimir proposes add a warning.
> 
> We MUST keep it because dropping it would break test plans that rely on it.
> 
> The simplest solution would be to ban Groovy GUI scripts and only allow
> Groovy files.
> 
> Remember that variable processing is not under the control of the test
> elements; it is done before the test element sees the script.

I suggest in this case that we keep allowing it but just add a warning in the gui.

We can still work on the checkbox as per Vladimir proposal

Banning groovy script from being embedded does not seem to be a good idea:
- embedding is nice because you have the syntax highligh, can modify it in gui and don't have to handle its copy on servers when running in non gui and particularly in distributed testing. And it would surely be an backward incompatibility that in my case breaks nearly 100% of my plans.

>Concerning backward compatibility. Trivaial solution is:
>- cache key is empty -> checkbox is cleared,
>- cache key is set -> checkbox is checked.
ok and keep the initial cache key (if set) when saving that will make the jmx file usable on a previous version of jmeter.

if the checkbox is checked then the real cache key will be computed from the MD5 of the script (inline gui).

the use of the cache key is not the best solution if the script use variables as the cache will grow (that's an LRU cache so it won't break the world) but it will work.

In jmeter 2.13, it's possible to set a cache key for a script with variables and it will fail silently as a wrong version of the script is executed

Did I mention that jmeter variables in script are bad ?

PR https://github.com/apache/jmeter/pull/83

Restore peace and justice to the universe.

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

Author: pmouawad
Date: Sun Feb 28 13:20:08 2016
New Revision: 1732732

URL: http://svn.apache.org/viewvc?rev=1732732&view=rev
Log:
Bug 56554 - JSR223 Test Element : Generate compilation cache key automatically
Update documentation
Remove also mention of old versions
Bugzilla Id: 56554

Modified:
    jmeter/trunk/xdocs/usermanual/component_reference.xml

Author: pmouawad
Date: Mon Mar 28 11:35:16 2016
New Revision: 1736874

URL: http://svn.apache.org/viewvc?rev=1736874&view=rev
Log:
Bug 56554 JSR223 Test Element : Generate compilation cache key automatically
Bugzilla Id: 56554

Modified:
    jmeter/trunk/src/components/org/apache/jmeter/assertions/JSR223AssertionResources.properties
    jmeter/trunk/src/components/org/apache/jmeter/assertions/JSR223AssertionResources_fr.properties

Author: pmouawad
Date: Mon Mar 28 11:42:17 2016
New Revision: 1736875

URL: http://svn.apache.org/viewvc?rev=1736875&view=rev
Log:
Bug 56554 JSR223 Test Element : Generate compilation cache key automatically
Change labels and description
Bugzilla Id: 56554

Modified:
    jmeter/trunk/src/components/org/apache/jmeter/extractor/JSR223PostProcessorResources.properties
    jmeter/trunk/src/components/org/apache/jmeter/extractor/JSR223PostProcessorResources_fr.properties
    jmeter/trunk/src/components/org/apache/jmeter/modifiers/JSR223PreProcessorResources.properties
    jmeter/trunk/src/components/org/apache/jmeter/modifiers/JSR223PreProcessorResources_fr.properties
    jmeter/trunk/src/components/org/apache/jmeter/timers/JSR223TimerResources.properties
    jmeter/trunk/src/components/org/apache/jmeter/timers/JSR223TimerResources_fr.properties
    jmeter/trunk/src/components/org/apache/jmeter/visualizers/JSR223ListenerResources.properties
    jmeter/trunk/src/components/org/apache/jmeter/visualizers/JSR223ListenerResources_fr.properties
    jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JSR223SamplerResources.properties
    jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JSR223SamplerResources_fr.properties

Author: pmouawad
Date: Thu Apr 21 21:12:07 2016
New Revision: 1740412

URL: http://svn.apache.org/viewvc?rev=1740412&view=rev
Log:
Bug 56554 JSR223 Test Element : Generate compilation cache key automatically. Contributed by Benoit Wiart (benoit dot wiart at gmail.com)
fix documentation issue
Bugzilla Id: 56554

Modified:
    jmeter/trunk/xdocs/usermanual/component_reference.xml

This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/3380