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

Reduce allocations when notify promises related to pending writes. #308

Merged
merged 2 commits into from
Apr 13, 2018
Merged

Reduce allocations when notify promises related to pending writes. #308

merged 2 commits into from
Apr 13, 2018

Conversation

normanmaurer
Copy link
Member

@normanmaurer normanmaurer commented Apr 12, 2018 •
edited
Loading

Motivation:

We returned an callback which needed to be executed by the caller. This will cause extra allocations and is not needed. We can just return a EventLoopPromise which will cascade the notification.

Modification:

Not return a closure but promise.

Result:

Less allocations.

@normanmaurer
Copy link
Member Author

Noticed this wile benchmarking...

weissi
weissi approved these changes Apr 12, 2018
View reviewed changes
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

removing allocations is great! thanks!

weissi
weissi reviewed Apr 12, 2018
View reviewed changes
Sources/NIO/PendingWritesManager.swift Outdated
/// - returns: A closure that the caller _needs_ to run which will fulfill the promises of the writes and a `OneWriteOperationResult` which indicates if we could write everything or not.
public mutating func didWrite(itemCount: Int, result writeResult: IOResult<Int>) -> (() -> Void, OneWriteOperationResult) {
/// - returns: An array of promises (that needs to be notified) of the writes and a `OneWriteOperationResult` which indicates if we could write everything or not.
public mutating func didWrite(itemCount: Int, result writeResult: IOResult<Int>) -> ([EventLoopPromise<Void>], OneWriteOperationResult) {
Copy link
Member

Choose a reason for hiding this comment

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

couldn't we actually return one EventLoopPromise<Void> and cascade all others into that?

weissi
weissi reviewed Apr 12, 2018
View reviewed changes
Sources/NIO/PendingWritesManager.swift Outdated
/// - returns: A closure that the caller _needs_ to run which will fulfill the promises of the writes and a `OneWriteOperationResult` which indicates if we could write everything or not.
public mutating func didWrite(itemCount: Int, result writeResult: IOResult<Int>) -> (() -> Void, OneWriteOperationResult) {
/// - returns: An array of promises (that needs to be notified) of the writes and a `OneWriteOperationResult` which indicates if we could write everything or not.
public mutating func didWrite(itemCount: Int, result writeResult: IOResult<Int>) -> ([EventLoopPromise<Void>], OneWriteOperationResult) {
var promises: [EventLoopPromise<Void>] = []
Copy link
Member

Choose a reason for hiding this comment

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

we could save this whole array if we return one EventLoopPromise

@Lukasa Lukasa added the semver/patch No public API change. label Apr 12, 2018
@normanmaurer
Copy link
Member Author

@weissi PTAL again..

@normanmaurer
Copy link
Member Author

Also when running our allocation tests this reduce the number of allocations per req by 2.

Also it improves other benchmarks (like echo)...

Before:

tcpkali -m xxxxxxxxxxx -r 100000000 -c 10 -T 30 127.0.0.1:9999
Destination: [127.0.0.1]:9999
Interface lo0 address [127.0.0.1]:0
Using interface lo0 to connect to [127.0.0.1]:9999
Ramped up to 10 connections.
Total data sent:     51370.0 MiB (53865310066 bytes)
Total data received: 51361.3 MiB (53856201954 bytes)
Bandwidth per channel: 2051.562⇅ Mbps (256445.2 kBps)
Aggregate bandwidth: 14359.719↓, 14362.147↑ Mbps
Packet rate estimate: 1336494.8↓, 1270857.5↑ (11↓, 22↑ TCP MSS/op)
Test duration: 30.004 s.

With the PR:

➜  ~ tcpkali -m xxxxxxxxxxx -r 100000000 -c 10 -T 30 127.0.0.1:9999
Destination: [127.0.0.1]:9999
Interface lo0 address [127.0.0.1]:0
Using interface lo0 to connect to [127.0.0.1]:9999
Ramped up to 10 connections.
Total data sent:     56375.4 MiB (59113928960 bytes)
Total data received: 56371.2 MiB (59109475485 bytes)
Bandwidth per channel: 3151.601⇅ Mbps (393950.1 kBps)
Aggregate bandwidth: 15757.410↓, 15758.598↑ Mbps
Packet rate estimate: 1474098.5↓, 1367098.5↑ (11↓, 21↑ TCP MSS/op)
Test duration: 30.0097 s.

weissi
weissi approved these changes Apr 12, 2018
View reviewed changes
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

nice, like this version much better, thank you!

@normanmaurer
Copy link
Member Author

Here are also the alloc differences with our test:

master:

1000_reqs_1_conn.total_allocations: 286308

With this PR:

1000_reqs_1_conn.total_allocations: 284306

Lukasa
Lukasa requested changes Apr 13, 2018
View reviewed changes
Sources/NIO/PendingWritesManager.swift Outdated
public mutating func didWrite(itemCount: Int, result writeResult: IOResult<Int>) -> (() -> Void, OneWriteOperationResult) {
var promises: [EventLoopPromise<Void>] = []
let fulfillPromises = { promises.forEach { $0.succeed(result: ()) } }
/// - returns: The promises, or nil, that cascades to all promises (that needs to be notified) of the writes and a `OneWriteOperationResult` which indicates if we could write everything or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's reword this: - returns: A tuple of a promise and a OneWriteResult. The promise is the first promise that needs to be notified of the write result. This promise will cascade the result to all other promises that need notifying. If no promises need to be notified, will be nil. The write result will indicate whether we were able to write everything or not.

@normanmaurer
Copy link
Member Author

@Lukasa addressed PTAL again

@normanmaurer
Reduce allocations when notify promises related to pending writes.
c85888f 
Motivation:

We returned an callback which needed to be executed by the caller. This will cause extra allocations and is not needed. We can just return a EventLoopPromise which will cascade the notification.

Modification:

Not return a closure but promise.

Result:

Less allocations.
@Lukasa Lukasa added this to the 1.5.0 milestone Apr 13, 2018
@Lukasa
Copy link
Contributor

Lukasa commented Apr 13, 2018

Go ahead and merge when ready.

@normanmaurer normanmaurer merged commit 7758140 into apple:master Apr 13, 2018
@normanmaurer normanmaurer deleted the reduce_allocs branch April 13, 2018 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weissi weissi weissi approved these changes

@Lukasa Lukasa Lukasa approved these changes

Assignees
No one assigned
Labels
semver/patch No public API change.
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@normanmaurer @Lukasa @weissi