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

Experiment with lock free AdaptiveExecutionStrategy #8762

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Oct 25, 2022

Removed the lock from the AdaptiveExecutionStrategy in favor of a BiInteger.

@sbordet pointed out that the AES lock is the most contended in the server when running cometd benchmarks. He pointed out that it could be implemented without a lock, so I've done that in this PR. @lorban can you benchmark this branch to see if there is any significant benefit? I'm hopeful as many paths through the CaS loops don't look at all the state and some don't need to change state.

The other thing to consider, is if the pending mechanism is needed at all? Perhaps the reserved thread pool is sufficient to stop stampedes? But as it is, pending handling is pretty simple... once you realize that it is now a counter rather than a boolean and it may go > 1 (two threads both start a pending at the same time) or < 0 (a started pending producer runs before the pending count is actually incremented).

Removed the lock from the AdaptiveExecutionStrategy in favour of a BiInteger.
@gregw
Copy link
Contributor Author

gregw commented Oct 31, 2022

@lorban an ETA for some benchmarks on this?

@lorban
Copy link
Contributor

lorban commented Oct 31, 2022

@gregw I've updated jetty-perf and jetty-load-generator to work with the latest 12.0.x changes.

You can expect some results in the coming hours.

@lorban
Copy link
Contributor

lorban commented Nov 1, 2022

I expected this change to have no measurable impact in the HTTP case, and I was right: you can like this change for code clarity but performance wise, this is equivalent to the locked version.

What I did not consider and surprised me is that this makes a measurable impact in the H2 case: the p99 latency goes down by 30% at the expense of a 25% higher max latency. And this made #8810 surface.

Overall, I think this is a good change that brings meaningful improvement to H2.

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minus one small nit.

PRODUCING, // There is an active producing thread.
REPRODUCING // There is an active producing thread and demand for more production.
}
static final int IDLE = 0; // No tasks or producers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants should be private.

@gregw gregw merged commit 7e88b82 into jetty-12.0.x Nov 1, 2022
@gregw
Copy link
Contributor Author

gregw commented Nov 1, 2022

@lorban @sbordet worth back porting this?

@gregw gregw deleted the jetty-12-lock-free-execution-strategy branch November 1, 2022 21:16
@lorban
Copy link
Contributor

lorban commented Nov 2, 2022

@gregw I'd say yes since it's a rather small and contained change.

@sbordet
Copy link
Contributor

sbordet commented Nov 2, 2022

@gregw yes please backport to 10.

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

Successfully merging this pull request may close these issues.

3 participants