Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partial fix for Throughput Shaping Timer #558

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pbaykalov
Copy link

This PR partially fixes erratic behaviour of Throughput Shaping Timer with low requested intensity.

This is the problem:
TST_MCVE
TST_MCVE.zip

Actual intensity might strongly depend on what exactly the requested intensity is and how many threads there are.

The timer is still not guaranteed to work correctly in specific but realistic situaitons.

if rps is less than 1
and if delay() is executed inside new second this condition will evaluate to false and cause double-release
potentially this can cause very long series of 1 RPS release if resonance is hit:
if ((total time for each sample to execute with all other thread actions) + msecPerReq ) % 1000 is close to 0 or 1000 then 1 RPS will fire for very long time for arbitrarily small RPS

this commit fixes this multiple release for small RPS but there are still problems if the requests are long enough

I will suggest better and simplier code in future commits
@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Merging #558 (c4580a8) into master (da6d10c) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##             master     #558   +/-   ##
=========================================
  Coverage     68.94%   68.95%           
  Complexity     2574     2574           
=========================================
  Files           230      230           
  Lines         15609    15609           
  Branches       1589     1589           
=========================================
+ Hits          10762    10763    +1     
+ Misses         4057     4055    -2     
- Partials        790      791    +1     
Impacted Files Coverage Δ
.../kg/apc/jmeter/timers/VariableThroughputTimer.java 82.24% <0.00%> (-0.60%) ⬇️
...ava/kg/apc/jmeter/jmxmon/JMXMonConnectionPool.java 82.81% <0.00%> (+3.12%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pbaykalov
Copy link
Author

What's the main reason for return 0; in delay()?

@undera
Copy link
Owner

undera commented Dec 18, 2022

What's the main reason for return 0; in delay()?

It's the default branch, in case we're not delivering enough hits. The meaning of 0 here is "do the next sample as soon as possible, don't add any delay".

@pbaykalov
Copy link
Author

What's the main reason for return 0; in delay()?

It's the default branch, in case we're not delivering enough hits. The meaning of 0 here is "do the next sample as soon as possible, don't add any delay".

It's not "default" it's the only return statement in delay.

@undera
Copy link
Owner

undera commented Dec 18, 2022

What's the main reason for return 0; in delay()?

It's the default branch, in case we're not delivering enough hits. The meaning of 0 here is "do the next sample as soon as possible, don't add any delay".

It's not "default" it's the only return statement in delay.

Ok, maybe we're talking about different places in code. Can you point to exact file and line that you're talking about?

@undera
Copy link
Owner

undera commented Dec 18, 2022

What's the main reason for return 0; in delay()?

It's the default branch, in case we're not delivering enough hits. The meaning of 0 here is "do the next sample as soon as possible, don't add any delay".

It's not "default" it's the only return statement in delay.

Ok, I see what you mean. So, the meaning of it is we tell JMeter that there should be no delay at all on its side, since our code has already did introduce the required delay with wait()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants