Skip to content
New issue

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

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

Already on GitHub? # to your account

Improve flatScheduleTask #3071

Closed
Lukasa opened this issue Jan 20, 2025 · 0 comments · Fixed by #3079
Closed

Improve flatScheduleTask #3071

Lukasa opened this issue Jan 20, 2025 · 0 comments · Fixed by #3079
Labels
area/performance Improvements to performance. good first issue Good for newcomers kind/enhancement Improvements to existing feature. size/S Small task. (A couple of hours of work.)

Comments

@Lukasa
Copy link
Contributor

Lukasa commented Jan 20, 2025

swift-nio has multiple implementations of flatScheduleTask which are all basically identical:

        let promise: EventLoopPromise<T> = self.makePromise(file: file, line: line)
        let scheduled = self.scheduleTask(deadline: deadline, task)

        scheduled.futureResult.flatMap { $0 }.cascade(to: promise)
        return .init(promise: promise, cancellationTask: { scheduled.cancel() })

Each of these is meaningfully different, e.g. in whether they take a deadline or a delay, or whether they require sendable closures and results.

@glbrntt has pointed out that the line scheduled.futureResult.flatMap { $0 }.cascade(to: promise), which is common to all of them, is potentially suboptimal. A better implementation would be:

scheduled.futureResult.whenComplete { result in
    switch result {
    case .success(let futureResult):
        promise.completeWith(futureResult)
    case .failure(let error):
        promise.fail(error)
    }
}

We should change the implementations, and then confirm that we do actually see an allocation reduction from the change. Assuming we do, we can safely merge it.

@Lukasa Lukasa added area/performance Improvements to performance. good first issue Good for newcomers kind/enhancement Improvements to existing feature. size/S Small task. (A couple of hours of work.) labels Jan 20, 2025
clintonpi added a commit to clintonpi/swift-nio that referenced this issue Jan 22, 2025
Motivation:

As pointed out in apple#3071, the `flatScheduleTask` implementations can be improved.

Modifications:

- Refactor the `flatScheduleTask` implementations to skip `flatMap` calls, which avoids creating an extra promise.

Result:

Potential reduction in the number of allocations in the package.
@glbrntt glbrntt linked a pull request Jan 22, 2025 that will close this issue
glbrntt added a commit that referenced this issue Jan 22, 2025
Motivation:

As pointed out in #3071, the `flatScheduleTask` implementations can be
improved.

Modifications:

- Refactor the `flatScheduleTask` implementations to skip `flatMap`
calls, which avoids creating an extra promise.
- As there is now a lower number of allocations, reduce the necessary
thresholds for the allocation tests.

Result:

Reduction in the number of allocations in the package.

---------

Co-authored-by: George Barnett <gbarnett@apple.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/performance Improvements to performance. good first issue Good for newcomers kind/enhancement Improvements to existing feature. size/S Small task. (A couple of hours of work.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant