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

Use fewer event loops when using io_uring as transport #5089

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

trustin
Copy link
Member

@trustin trustin commented Jul 31, 2023

Motivation:

io_uring is known to have much smaller system call overhead than other
transport types such as NIO, epoll and Kqueue. Therefore, we should
use fewer event loop threads when using io_uring.

Modifications:

  • Changed FlagsProvider.numCommonWorkers() to require TransportType
  • Made DefaultFlagsProvider.numCommonWorkers() not double the number
    of CPU cores when the current transport type is IO_URING

Result:

  • A user doesn't have to manually specify the optimal event loop count
    when using io_uring.

Motivation:

`io_uring` is known to have much smaller system call overhead than other
transport types such as NIO, `epoll` and Kqueue. Therefore, we should
use fewer event loop threads when using `io_uring`.

Modifications:

- Changed `FlagsProvider.numCommonWorkers()` to require `TransportType`
- Made `DefaultFlagsProvider.numCommonWorkers()` not double the number
  of CPU cores when the current transport type is `IO_URING`

Result:

- A user doesn't have to manually specify the optimal event loop count
  when using `io_uring`.
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.01% ⚠️

Comparison is base (32568f9) 74.19% compared to head (a79eee8) 74.19%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5089      +/-   ##
============================================
- Coverage     74.19%   74.19%   -0.01%     
- Complexity    19469    19470       +1     
============================================
  Files          1672     1672              
  Lines         71749    71752       +3     
  Branches       9189     9190       +1     
============================================
  Hits          53237    53237              
- Misses        14186    14187       +1     
- Partials       4326     4328       +2     
Files Changed Coverage Δ
...ava/com/linecorp/armeria/common/FlagsProvider.java 96.77% <ø> (ø)
...rp/armeria/common/SystemPropertyFlagsProvider.java 63.69% <ø> (ø)
.../linecorp/armeria/common/DefaultFlagsProvider.java 86.41% <50.00%> (-2.19%) ⬇️
...c/main/java/com/linecorp/armeria/common/Flags.java 73.96% <50.00%> (+0.07%) ⬆️

... and 20 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

public Integer numCommonWorkers() {
final int defaultNumCpuCores = Runtime.getRuntime().availableProcessors();
return defaultNumCpuCores * 2;
public Integer numCommonWorkers(TransportType transportType) {
Copy link
Contributor

@ikhoon ikhoon Aug 2, 2023

Choose a reason for hiding this comment

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

Optional) There is a similar case where a flag depends on another flag to resolve the final value like this. #5029 (comment)
What do you think of a getter method for the composed FlagsProvider and set it to SystemPropertyFlagsProvider?

For example:

interface FlagsProvider {
    // Needs a better name.
    default FlagsProvider composedFlagsProvider() { return null; }
}

...
SystemPropertyFlagsProvider systemProvider = new SystemPropertyFlagsProvider();
final List<FlagsProvider> flagsProviders = ServiceLoader.load(...);
flagsProviders.add(0, systemProvider);
flagsProviders.add(DefaultFlagsProvider.INSTANCE);
systemProvider.setComposedProvider(flagsProviders);

public Integer numCommonWorkers() {
    if (composedFlagsProvider().transportType() == TransportType.IO_URING) {
        ...
    }
}

One concern is the circular reference between flags. If it looks overkill or dangerous to use, feel free to ignore this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds a little bit dangerous to me because it's fairly easy to trigger circular dependency as you said. We could revisit this when we end up with the situation that really needs this.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @trustin! 🚀

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Looks straightforward 👍 Thanks @trustin 🙇 👍 🙇

@jrhee17 jrhee17 merged commit d294a66 into main Aug 4, 2023
14 of 15 checks passed
@jrhee17 jrhee17 deleted the io_uring_defaults branch August 4, 2023 05:19
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.

None yet

3 participants