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.

Sign up for GitHub

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

Jump to bottom

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
@Lukasa Lukasa mentioned this issue Jan 20, 2025
Merged
clintonpi added a commit to clintonpi/swift-nio that referenced this issue Jan 22, 2025
@clintonpi
Improve flatScheduleTask
dc5742e 
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.
@clintonpi clintonpi mentioned this issue Jan 22, 2025
Merged
@glbrntt glbrntt linked a pull request Jan 22, 2025 that will close this issue
Improve flatScheduleTask #3079
Merged
glbrntt added a commit that referenced this issue Jan 22, 2025
@clintonpi @glbrntt
Improve flatScheduleTask (#3079)
b14012b 
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve flatScheduleTask clintonpi/swift-nio
1 participant
@Lukasa