PseudoHeaderFields adopting CoW to reduce size

[guoye-zhang]

HTTPRequest size 288 -> 16
HTTPResponse size 80 -> 16

rdar://144520456

[Lukasa]

Left a small note around the _modify accessor that may be worth some consideration.

I like the approach of having the two object references side-by-side, but I want to make sure we're considering the tradeoff here. The other approach would have been to move everything, including the HTTPFields, into the backing class.

If we did that, the cost of copying this object goes down further (it's just a single call to swift_retain/swift_release instead of two per copy). However, the cost of accessing the fields goes up slightly if it's done rarely, as we have a double-indirection to get to them.

I'd love to hear your thoughts about the choice between those two @guoye-zhang.

[Lukasa]

A bit of a nit, but these accessors probably want to either replace set with _modify or to use @inlinable. Otherwise minor modifications can risk CoWing the underlying strings where it isn't necessary to do so.

This isn't a huge deal as I don't think there's a lot of minor string modifications happening this way (pretty rare to do request.path.value += "/something/else") but it'd be nice to avoid the CoW in that case.

[guoye-zhang]

I'm not very familiar with _modify. Does this look reasonable?

            _modify {
                if !isKnownUniquelyReferenced(&self._storage) {
                    self._storage = self._storage.copy()
                }
                yield &self._storage.path
                if let name = self._storage.path?.name {
                    precondition(name == .path, "Cannot change pseudo-header field name")
                }
            }

[Lukasa]

I believe that will be fine, though _modify is extremely sensitive to the exact nature of its flow control. I recommend actually building this under -O and checking that there isn't a call to malloc or similar there.

[guoye-zhang]

There is a call to malloc after this change. Not sure how to get rid of it.

libHTTPTypes.dylib.zip

[Lukasa]

Will take a look

[Lukasa]

Same note on these accessors re _modify.

[guoye-zhang]

I like the approach of having the two object references side-by-side, but I want to make sure we're considering the tradeoff here. The other approach would have been to move everything, including the HTTPFields, into the backing class.

Initially I thought that we were limited by the current API shape since PseudoHeaderFields is a distinct type that can be operated on its own (e.g. request1.pseudoHeaderFields = request2.pseudoHeaderFields). But now that I think more about it, it is definitely possible to do. Also I wasn't sure about nesting 2 levels of CoW for headerFields but it should be fine, too.

I took the easier approach to implement them as separate CoW structs. I think 2-words is small enough like a Swift String.

[ekinnear]

It's interesting that we keep coming back to this pattern, but if that's the right way to do it, then 👍.
Approved pending the other discussion.

[Lukasa]

I took the easier approach to implement them as separate CoW structs. I think 2-words is small enough like a Swift String.

Ah yeah, I'm not concerned about the size, only about the cost of copying. But agreed that the current design is fine.