get IOError down to 24 bytes (#1355)

[realdoug]

resolves #1355 shaving off 2 bytes from IOError struct

Modifications:

This one had a bit of a curveball. Replacing StaticString shaves off one byte. The other extra byte is saved by switching the order that the enum and Int32 properties are declared. Seems like a compiler bug to me.

For example:

struct Good {
    enum FailureDescription {
        case function(String)
        case reason(String)
    }

    let reason: FailureDescription
    let errnoCode: Int32
}

struct Bad {
    enum FailureDescription {
        case function(String)
        case reason(String)
    }

    let errnoCode: Int32
    let reason: FailureDescription
}

print(MemoryLayout<Good>.size)
print(MemoryLayout<Bad>.size)

// 24
// 25

I'm curious now why that might be.

Result:

New condition:

print(MemoryLayout<IOError>.size) // 24

[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?

[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]

Hmm, I wonder where the enum discriminator went in the “good” case. The only excuse for shrinking this by one byte when FailureDescription is clearly 20 bytes wide is because the enum discriminator got stuffed somewhere in the Good case that it did not in the Bad, but I can’t really see why that optimisation wasn’t available in both cases.

[Lukasa]

Sadly, I don’t think this change is quite right: StaticString and String are different types, which means this has changed the public API surface of the module in a backward-incompatible way (i.e. it is possible that some code used to compile and now does not).

Sadly that particular change is not available to us: we’ll need to find another approach here.

[weissi]

First of all, thanks very much for this PR @realdoug ! As @Lukasa points out this is public API breaking right now. That's my mistake, I didn't quite appreciate how much we exported in our API here but I have a proposal how we can fix it all and remain API stable: Internally we just stop using the enum case that uses the StaticString and we can convert

public let reason: FailureDescription

to

private let failureDescription: String

public var reason: FailureDescription {
    return .reason(self.failureDescription
}

we should then also deprecate this init

public init(errnoCode: Int32, function: StaticString)

and rewrite it as

@available(*, deprecated, renamed: "init(errnoCode:reason:)")
public init(errnoCode: Int32, function: StaticString) {
    self.failureDescription = "\(function)"
}

[realdoug]

Gotcha. I figured you may want to paper over it somehow. Pushed a new version w/ updates @Lukasa @weissi ; Int32 + String results in the correct 20 bytes.

It's worth noting that this implementation loses the information distinguishing between reason/function. Perhaps that's ok, though if so, the enum & the computed property that returns it should be deprecated as well, right?

Alternatively, I could add a private Bool and conditionally return the correct FailureDescription value. Or perhaps the correct solution is both: use a private bool and deprecate both the enum and the computed prop that returns it.

Edit: realized this isn't possible since String -> StatcString conversion is impossible.

[realdoug]

Also, the deprecation causes many tests to emit warnings, which need to be cleaned up, but I wanted to validate the approach first.

[weissi]

@realdoug nice once! Great job! The information loss is okay, the old one was just there because in Swift before 5.0, Strings were very bad so StaticString made sense.

Regarding the warnings, can’t we just make NIO use reason: everywhere instead of function: ? I think that could be done with a big ol’ find&replace :).

We might want to add one single test which tests that the old function: still works. You can just deprecate the tests (you’ll find examples) then it won’t complain about it using deprecated functionality

[weissi]

@swift-nio-bot test this please

[weissi]

Thanks so much! I’m happy with this , we just need to adjust the copyright years and then I think this is good to go

[weissi]

CI passed, that failure is a spurious perf test. So it’s all good from my PoV apart from the copyright years

[realdoug]

ah my bad @weissi copyright year is fixed now.

[weissi]

@swift-nio-bot test this please

[weissi]

@swift-nio-bot test this please

[weissi]

Thanks so much! This looks great

[weissi]

@swift-nio-bot test this please

[Lukasa]

This looks great, thanks so much @realdoug!

[weissi]

@swift-nio-bot test this please

[weissi]

@swift-nio-bot test this please

[realdoug]

Awesome!