Removing unnecessary lazy calls.

[OliverLetterer]

/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift had unnecessary lazy calls in it. Since the results of the lazy calls are immediately consumed, using lazy here introduces an unnecessary performance overhead!

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[Lukasa]

Thanks for this PR! However, it's not the case that the .lazy has no use because the call is immediately consumed without overhead. Without .lazy, .map will allocate and return an Array. That Array is a performance cost, and we don't want to materialise it.

[OliverLetterer]

That is simply not correct!

[dnadoba]

You have changed lazy in two places. Lets first talk about about the Dictionary.init(_:uniquingKeysWith:) at line 607:

async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift

Lines 606 to 611 in 2adca4b

	let startingRequiredEventLoopConnectionCount = Dictionary(
	self.connections[self.overflowIndex..<self.connections.endIndex].lazy.map {
	($0.eventLoop.id, 1)
	},
	uniquingKeysWith: +
	)

I have slightly modified the test as I wasn't sure how you have created allobjects but I can't reproduce your test results. Here is my modified version:

@inline(never)
func blackHole(_ value: some Any) {}

func lazy() {
    let allobjects = Array(repeating: 1, count: 100)
    var counter = 0
    let duration = ContinuousClock().measure {
        for _ in 0..<1_000_000 {
            counter += Dictionary(allobjects[0..<40].lazy.map({ ($0, 1) }), uniquingKeysWith: +).values.first!
        }
    }
    blackHole(counter)
    print ("lazy", duration)
}

func eager() {
    let allobjects = Array(repeating: 1, count: 100)
    var counter = 0
    let duration = ContinuousClock().measure {
        for _ in 0..<1_000_000 {
            counter += Dictionary(allobjects[0..<40].map({ ($0, 1) }), uniquingKeysWith: +).values.first!
        }
    }
    blackHole(counter)
    print ("eager", duration)
}

lazy()
eager()

which prints in release mode with swift-5.7-DEVELOPMENT-SNAPSHOT-2022-07-17-a:

lazy 5.722392167000001 seconds
eager 6.738160625 seconds

This makes perfect sense as no intermediate array is allocated because Dictionary.init(_:uniquingKeysWith:) takes some Sequence as it's input.

---

However, I think you are right about the second place where you have changed lazy:

async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift

Lines 612 to 623 in 2adca4b

	var connectionToCreate = requiredEventLoopOfPendingRequests
	.flatMap { eventLoop, requestCount -> [(Connection.ID, EventLoop)] in
	// We need a connection for each queued request with a required event loop.
	// Therefore, we look how many request we have queued for a given `eventLoop` and
	// how many connections we are already starting on the given `eventLoop`.
	// If we have not enough, we will create additional connections to have at least
	// on connection per request.
	let connectionsToStart = requestCount - startingRequiredEventLoopConnectionCount[eventLoop.id, default: 0]
	return stride(from: 0, to: connectionsToStart, by: 1).lazy.map { _ in
	(self.createNewOverflowConnection(on: eventLoop), eventLoop)
	}
	}

We have manually specified the return type as being an Array, we sadly do allocate an intermediate Array.
But flatMap has an overload which actually accepts some Sequence as the return type from the closure and hence eliminates the intermediate allocation too: https://developer.apple.com/documentation/swift/sequence/flatmap(_:)-jo2y
Using this isn't straight forward though because .lazy.map(_:) will now take an escaping closure and we make a mutating call to self.

The solution to this is a bit ugly as we need to use withoutActuallyEscaping and would look something like this:

var connectionToCreate = withoutActuallyEscaping({ (eventLoop: EventLoop) -> HTTPConnectionPool.Connection.ID in
    self.createNewOverflowConnection(on: eventLoop)
}) { createNewOverflowConnectionOnEventLoop in
    requiredEventLoopOfPendingRequests
       .flatMap { eventLoop, requestCount -> LazyMapSequence<LazySequence<StrideTo<Int>>.Elements, (HTTPConnectionPool.Connection.ID, EventLoop)> in
           // We need a connection for each queued request with a required event loop.
           // Therefore, we look how many request we have queued for a given `eventLoop` and
           // how many connections we are already starting on the given `eventLoop`.
           // If we have not enough, we will create additional connections to have at least
           // on connection per request.
           let connectionsToStart = requestCount - startingRequiredEventLoopConnectionCount[eventLoop.id, default: 0]
           
           return stride(from: 0, to: connectionsToStart, by: 1).lazy.map { _ in
               (createNewOverflowConnectionOnEventLoop(eventLoop), eventLoop)
           }
       }
}

I haven't tested the performance but I'm quite confident that this will improve performance too as we do not allocate an intermediate array.

As this makes the code a bit harder to follow I ended up not using the withoutActuallyEscaping trick. The lazy is just an artifact I forgot while reverting the change. As lazy has no effect here, we could just remove it. @Lukasa what do you think? Is it worth the complexity to use the non-allocating withoutActuallyEscaping trick or should we just remove lazy?

[Lukasa]

In the second case you're right that .lazy is entirely unnecessary. Sorry about the confusion there, that's my fault. As constructed, the .lazy achieves nothing, because we're specifying that we're returning an Array. I definitely don't think we gain enough to justify using withoutActuallyEscaping, which is a pattern I generally dislike.

However, in the case of Dictionary construction there remains insufficient evidence to make the case that this flow optimises the intermediate Array away. That allocation is extraordinarily painful, and it's worth removing even though .lazy is a substantially slower way to iterate.

[Lukasa]

As a sidebar, if we really wanted to make this faster the way to do it is to replace this functional code that feels like it wants to use .lazy with imperative iteration. Swift is substantially better at optimising the latter (due to the absence of escaping closures).