Bug 56554 - JSR223 Test Element : Generate compilation cache key automatically
Summary: JSR223 Test Element : Generate compilation cache key automatically
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 2.13
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-22 12:20 UTC by Vladimir Ryabtsev
Modified: 2016-04-21 21:13 UTC (History)
2 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimir Ryabtsev 2014-05-22 12:20:17 UTC
From documentation we know that engine tries to compile script in JSR223 if compilation cache key is set. But why do we need take care about its uniqueness? Why can't JMeter automatically generate random string for this field (or, for example, hash from Script Text)?

My proposal is:
1. Remove Compilation Cache Key field.
2. Add Compile flag, and make it available only if choosen Scripting Engine implement Compilable interface.
3. In case Compilable flag is set, generate cache key automatically.
Comment 1 benoit.wiart 2016-01-07 08:55:48 UTC
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.
Comment 2 Sebb 2016-01-07 11:13:18 UTC
(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.
Comment 3 benoit.wiart 2016-01-08 17:00:26 UTC
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
Comment 4 Sebb 2016-01-08 17:36:25 UTC
(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.
Comment 5 Felix Schumacher 2016-01-09 18:56:13 UTC
(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.
Comment 6 benoit.wiart 2016-01-12 23:21:52 UTC
@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
Comment 7 Sebb 2016-01-13 00:46:04 UTC
(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.
Comment 8 Vladimir Ryabtsev 2016-01-13 09:07:15 UTC
(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.
Comment 9 Vladimir Ryabtsev 2016-01-13 09:16:36 UTC
(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.
Comment 10 Sebb 2016-01-13 10:29:20 UTC
(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.
Comment 11 Sebb 2016-01-13 10:35:17 UTC
(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.
Comment 12 Vladimir Ryabtsev 2016-01-13 13:34:36 UTC
(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.
Comment 13 Felix Schumacher 2016-01-13 21:07:00 UTC
(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.
Comment 14 Philippe Mouawad 2016-01-13 21:35:51 UTC
(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.
Comment 15 Sebb 2016-01-14 01:45:45 UTC
(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.
Comment 16 Philippe Mouawad 2016-01-14 06:41:36 UTC
(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.
Comment 17 benoit.wiart 2016-01-14 23:21:23 UTC
>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 ?
Comment 18 benoit.wiart 2016-01-18 21:17:03 UTC
PR https://github.com/apache/jmeter/pull/83

Restore peace and justice to the universe.
Comment 19 Philippe Mouawad 2016-01-25 20:48:14 UTC
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
Comment 20 Philippe Mouawad 2016-02-28 13:20:40 UTC
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
Comment 21 Philippe Mouawad 2016-03-28 11:42:34 UTC
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
Comment 22 Philippe Mouawad 2016-04-21 21:13:10 UTC
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