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

consolidate concurrency and rate limiting critical sections #1748

Open
6 tasks
jshook opened this issue Dec 22, 2023 · 0 comments
Open
6 tasks

consolidate concurrency and rate limiting critical sections #1748

jshook opened this issue Dec 22, 2023 · 0 comments
Assignees

Comments

@jshook
Copy link
Contributor

jshook commented Dec 22, 2023

With virtual threads, it makes more sense to launch a new virtual thread per operation, given the variety of tasks that NB must juggle for an activity. In nearly every case, there is some IO contended resource or activity. Alternately, striking a balance between native threads and virtual threads is not worthwhile at this level since it adds complexity and would be non-trivial to tune across different situations.

Given that we can start tasks very cheaply with virtual threads, the value of keeping native threads around all but disappears. The stateful task (op) tracking and sharing data can be handed to a new virtual thread just as easily as when new threads were constructed before. These structures are fixtures in the execution context already, and add no new burden to virtual thread life cycle.

This change proposes to consolidate the concurrency limit and the rate limit to a single call within SimRate, as there is already a blocking call path there which can do both much more efficiently than before.

  • A task should not start until
    • it is allowed to start according to the rate limit
    • a unit of concurrency is available to borrow

Both of these conditions is marshaled by a semaphore. The rate limit uses the semaphore count as a leaky bucket, and the concurrency limit simply uses a semaphore in the classic sense for grants.

The concurrency interface should implement AutoCloseable. In the case that the rate limiter is inactive, as for no rate set, an alternate implementation should be substituted which no-ops the rate portion.

Dynamic changing of the rate as well as the concurrency should be supported. When the concurrency is lowered, pending operations should not be stopped, but new operations should not be started unless/until the concurrency semaphore would otherwise allow.

The amount of code that can be removed is substantial after this is supported. The recommended steps to implement are:

  • Implement a concurrency limiter which is separate from SimRate with a comprehensive set of unit tests.
  • Add SimRate support for concurrency with a comprehensive set of unit tests.
  • Implement a variant of the ActivityExecutor which delegates concurrency management to SimRate.
  • Make the implementation of the ActivityExecutor selectable via enum and configuration option.
  • Verify performance deltas and corner case scenarios between the implementations.
  • Remove the previous ActivityExecutor implementation and remove the option.
@jshook jshook added this to the P0 - Advanced Analysis milestone Dec 22, 2023
@jshook jshook self-assigned this Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant