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

allow setting MTELG.singleton as Swift Concurrency executor #2564

Merged
merged 3 commits into from Jan 2, 2024

Conversation

weissi
Copy link
Member

@weissi weissi commented Oct 20, 2023

Motivation:

There are situations where the currently necessary thread switches between Swift Concurrency's default global executor and a place that can do I/O (such as Swift NIO's EventLoops) can be very detrimental to the overall system performance.

The right solution is something akin to upcoming the Task Executor preference proposal but for now (Swift 5.9) there is another solution. Instead of separating the threads of Swift Concurrency & SwiftNIO entirely we can replace Swift Concurrency's default global executor by a more powerful executor: SwiftNIO's EventLoopGroups.

The result is that almost all non-MainActor async functions will then run on SwiftNIO's EventLoops. That means if those have to perform I/O they don't necessarily have to switch to a separate I/O thread (such as NIO's EventLoops) anymore because they might be already there.

Be mindful however: This approach might not work well for all workloads and is a global setting.

Some more information about this can be found in:

Modifications:

Provide a NIOSingletons.unsafeTryInstallSingletonPosixEventLoopGroupAsConcurrencyGlobalExecutor() method that installs MultiThreadedEventLoopGroup.singleton as Swift Concurrency's global executor.

Result:

Faster I/O on Swift Concurrency.

@weissi weissi force-pushed the jw-concurrency-takeover branch 3 times, most recently from a6c8df3 to d9bb991 Compare October 20, 2023 11:46
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Thanks for all the checks before allowing the hooking. This looks as okey as it can be heh 😉

let’s get task executors in soon! :-)


private protocol SilenceWarning {
@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
func enqueue(_ job: UnownedJob)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah huh fun workaround. Okey

@weissi
Copy link
Member Author

weissi commented Oct 20, 2023

let’s get task executors in soon! :-)

Yes please

@weissi weissi force-pushed the jw-concurrency-takeover branch 2 times, most recently from 78269e2 to 7067f30 Compare October 20, 2023 15:38
@weissi weissi requested a review from Lukasa October 23, 2023 11:18
@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Nov 6, 2023
_haveWeTakenOverTheConcurrencyPool.compareExchange(
expected: false,
desired: true,
ordering: .relaxed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ordering: .relaxed
ordering: .acquiringAndReleasing

This is probably safe without using an acquiring load here due to the dlsym call that follows it, but semantically this should be an acquiring load. Logically then it also has to be a releasing store because we're synchronising with ourselves!

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lukasa I think you're mixing up the two variables. This is just the guard, no other memory reads/writes are ordered with this guy. This is correct not because of the dlsym but because of the guard.

You need non-.relaxed if you care about the order of other memory reads/writes but we don't

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we don't even need the guard. So yeah, any order of memory operations is completely fine here.

guard concurrencyEnqueueGlobalHookAtomic.compareExchange(
expected: nil,
desired: enqueueOnNIOPtr.pointee,
ordering: .relaxed
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly this should probably be a releasing ordering. Again, .enqueue should be an appropriate compiler barrier so arguably unnecessary, but I'd rather we did this right.

Copy link
Member Author

Choose a reason for hiding this comment

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

This one's a little more tricky but still, this is a singular memory operation, it doesn't matter when we do to the other (non-dependent) ones here: This can be .relaxed. Any order of non-dependent memory operations works for us here so this is fine too

Copy link
Contributor

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

LGTM!

@weissi
Copy link
Member Author

weissi commented Dec 18, 2023

@swift-nio-bot test this please

@weissi weissi enabled auto-merge (squash) December 18, 2023 16:04
@weissi weissi merged commit 5c668eb into apple:main Jan 2, 2024
8 of 10 checks passed
@weissi weissi deleted the jw-concurrency-takeover branch January 3, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants