You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by "vlsi (via GitHub)" <gi...@apache.org> on 2023/03/07 11:01:18 UTC
[GitHub] [jmeter] vlsi opened a new pull request, #5788: feat: add property to disable FunctionProperty caching
vlsi opened a new pull request, #5788:
URL: https://github.com/apache/jmeter/pull/5788
## Description
Cache function execution during test execution.
By default, JMeter caches function properties during a test iteration, however, it might cause unexpected results when a component is shared across threads and the expression depends on the thread variables.
I suggest we disable the caching behaviour would likely be disabled in the upcoming versions.
The PR does not change the existing behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi merged pull request #5788: feat: add property to disable FunctionProperty caching
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi merged PR #5788:
URL: https://github.com/apache/jmeter/pull/5788
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5788: feat: add property to disable FunctionProperty caching
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5788:
URL: https://github.com/apache/jmeter/pull/5788#issuecomment-1536040128
Frankly speaking, I incline we should change the default to "do not cache based on iteration number only" as the previous behavior (==cache based on iteration number) was introducing silent and hard-to-notice bugs, so I would alter the default to "do not cache".
If somebody still needs the cache, they could activate it back, however, I expect we would still remove the setting sometime in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #5788: feat: add property to disable FunctionProperty caching
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #5788:
URL: https://github.com/apache/jmeter/pull/5788#discussion_r1148544681
##########
xdocs/usermanual/properties_reference.xml:
##########
@@ -1339,6 +1339,14 @@ JMETER-SERVER</source>
ORO PatternCacheLRU size.<br/>
Defaults to: <code>1000</code>
</property>
+<property name="function.cache.per.iteration">
+ <p>Cache function execution during test execution.</p>
+ <p>By default, JMeter caches function properties during a test iteration, however,
+ it might cause unexpected results when a component is shared across threads and the expression depends on
+ the thread variables.</p>
+ <note>The caching behaviour would likely be disabled in the upcoming versions</note>
Review Comment:
I have not done explicit comparisons, however, I saw no perf difference when it comes to my regular load testing results
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #5788: feat: add property to disable FunctionProperty caching
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #5788:
URL: https://github.com/apache/jmeter/pull/5788#discussion_r1186450333
##########
src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/BeanShellSampler.java:
##########
@@ -102,14 +102,6 @@ public SampleResult sample(Entry e)// Entry tends to be ignored ...
return res;
}
try {
- String request = getScript();
Review Comment:
@FSchumacher , it turns out this caused two executions of `getScript` for `BeanShellSampler`.
One was to pass the script for `res.setSamplerData`, and the second `getScript` was in `processFileOrScript` for actual BSH execution.
I guess that is unexpected. However, the fix does not seem to be trivial: if I change `processFileOrScript` signature, it breaks backward compatibility because others might have overridden the method.
On the other hand, `processFileOrScript` does not expose `script` and `fileName`, so it is not clear how to get that information out.
For now, I added a new parameter to `processFileOrScript` so it calls `setSamplerData` as needed. I think it is unlikely that people override `processFileOrScript`. I checked GitHub search, and I found no cases for overriding `processFileOrScript`.
If you are ok with that, I think we need to make the same changes for JSR223 samplers. They have exactly the same issue, and it can call `getScript()` several times during execution as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on a diff in pull request #5788: feat: add property to disable FunctionProperty caching
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #5788:
URL: https://github.com/apache/jmeter/pull/5788#discussion_r1148544589
##########
xdocs/usermanual/properties_reference.xml:
##########
@@ -1339,6 +1339,14 @@ JMETER-SERVER</source>
ORO PatternCacheLRU size.<br/>
Defaults to: <code>1000</code>
</property>
+<property name="function.cache.per.iteration">
+ <p>Cache function execution during test execution.</p>
+ <p>By default, JMeter caches function properties during a test iteration, however,
+ it might cause unexpected results when a component is shared across threads and the expression depends on
+ the thread variables.</p>
+ <note>The caching behaviour would likely be disabled in the upcoming versions</note>
Review Comment:
I believe the cache creates functional issues: it caches the values, and it does not invalidate the cache. In other words, it assumes "the computed value depends only on the iteration number", which, in my opinion, is quite poor approximation.
So my main goal here is to establish correctness, and if there are any performance implications, then they should be healed separately
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] FSchumacher commented on a diff in pull request #5788: feat: add property to disable FunctionProperty caching
Posted by "FSchumacher (via GitHub)" <gi...@apache.org>.
FSchumacher commented on code in PR #5788:
URL: https://github.com/apache/jmeter/pull/5788#discussion_r1148543694
##########
xdocs/usermanual/properties_reference.xml:
##########
@@ -1339,6 +1339,14 @@ JMETER-SERVER</source>
ORO PatternCacheLRU size.<br/>
Defaults to: <code>1000</code>
</property>
+<property name="function.cache.per.iteration">
+ <p>Cache function execution during test execution.</p>
+ <p>By default, JMeter caches function properties during a test iteration, however,
+ it might cause unexpected results when a component is shared across threads and the expression depends on
+ the thread variables.</p>
+ <note>The caching behaviour would likely be disabled in the upcoming versions</note>
Review Comment:
```suggestion
<note>The caching behaviour will likely be disabled in an upcoming version</note>
```
Looks good to me.
Out of curiosity, have you done performance comparisons? I would think, that in most cases on modern CPUs the cache is not that important anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org