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.

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

Add custom backing storage to ByteBuffer #1218

Closed
Joannis opened this issue Nov 3, 2019 · 6 comments
Closed

Add custom backing storage to ByteBuffer #1218

Joannis opened this issue Nov 3, 2019 · 6 comments

Comments

@Joannis
Copy link

Joannis commented Nov 3, 2019

ByteBuffer's API is great for common in-memory binary data storage and for parsing/serializing data.
One of the problem that I find in every single project I have is the poor performance of ByteBuffer's backing storage which Swift is retaining/releasing every single API call. I'd love to have a data type such as UnsafeByteBuffer and UnsafeMutableByteBuffer which is the same API wrapped around a manually allocated/deallocated buffer.

I'm very sure that this would open up many opportunities for optimizing current applications such as Database drivers, JSON libraries and templating libraries. Even if those oppurtunities are there now, it'd still be a great improvement in the ease of doing said operations.

Databases

Many databases will, for example, return a contiguous buffer with many separate entities. For each entity, an encoder will need to be created which decodes the info of a single entity's row/document into a struct/class. I

struct DatabasePacket {
    let buffer: ByteBuffer

    var rows: Int { 
        // computed property to return the amount of rows read from the buffer as indicated by a packet header
    }
}

let packet: DatabasePacket = ...
var dataModels = [DataModel]() // Of course you'd reserve capacity etc...

for _ in 0..<packet.rows {
    // A 
    let rowLength = packet.buffer.readInteger(as: Int32.self)
    let unsafeByteBuffer = packet.readUnsafeByteBuffer(length: Int(rowLength))

    let model = try DatabaseRowDecoder().decode(DataModel.self, from: unsafeByteBuffer)
    dataModels.append(model)
}

UnsafeByteBuffer Pseudo Implementation

I'd directly refer to the pointer and provide an API for reading the data from that. Like ByteBuffer but without wrapping a class for auto-deallocation. It also wouldn't allow mutations and therefore mutation.

struct UnsafeByteBuffer {
    private let pointer: UnsafeRawPointer
    private var readerIndex: Int
    private let totalBytes: Int
}

UnsafeMutableByteBuffer Pseudo Implementation

I'd directly refer to the pointer again to provide an API for reading the data from that. Again without wrapping a class for auto-deallocation. But it would allow mutations, but would fatalError or at least do an assertionFailure if you try to access data outside of bounds.

struct UnsafeMutableByteBuffer {
    private let pointer: UnsafeMutableRawPointer
    public private(set) var readerIndex: Int
    public private(set) var writerIndex: Int
    private let totalBytes: Int
    private var isDeallocated = false

    mutating func deallocate() {
        if isDeallocated { return }
        isDeallocated = true
        pointer.deallocate()
    }
}

The goal of this type is not to read data efficiently, but to modify current buffers manually and/or to provide a useful interface. It might support auto-expansion using mutating methods. But allocation/deallocation is 100% within the developer's control.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 3, 2019

Thanks for this suggestion!

I am extremely disinclined to add an unsafe data type so prominently to NIO’s API. Specifically, I think that we should view any instance where Swift is not able to obtain an optimisation result as good as the unsafe equivalent as a Swift bug that should be reported upstream.

In your sample code the reference counting overhead should be pretty low; one atomic add per field, which ought not to be a drastic amount. I’d be interested in knowing how many more retains and releases you are seeing, as if that number is drastically higher this might be a good candidate for a benchmark.

@Joannis
Copy link
Author

Joannis commented Nov 3, 2019

https://github.com/Joannis/WIPTemplatingLib

This codebase has an implementation of what I proposed. My current test runs 10_000 iterations. Each set of 10k iterations runs in ~20ms:
[0.023683, 0.020158, 0.019228, 0.018847, 0.017709, 0.018982, 0.019211, 0.019213, 0.018431, 0.017715]

Before this the amount of time spend was 2.5-4x higher (50-80ms) no matter what I tried. All of that is retains/releases.

@Joannis
Copy link
Author

Joannis commented Nov 3, 2019

The fastest I could get it with SwiftNIO ByteBuffer: https://github.com/Joannis/WIPTemplatingLib/tree/slow

[0.061794, 0.059064, 0.055886, 0.056984, 0.059281, 0.055018, 0.052921, 0.053596, 0.052907, 0.055347]

image

@Lukasa
Copy link
Contributor

Lukasa commented Nov 4, 2019

So in my sample I halved your runtime by applying 5 @inlinable annotations to NIO (and in practice I think you only needed 3), as shown in #1220. I'm going to keep checking but I think this elides basically all the retain/release operations in your code, which suggests that moving to unsafe code is not necessary here.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 4, 2019

See also: https://bugs.swift.org/browse/SR-11706.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 5, 2019

There's one more retain/release happening in your withSlice function with the optimisations applied. I haven't nailed down exactly what's being retained here: it doesn't seem to be the ByteBuffer backing storage, so I suspect the issue is going to be related to the closure context. Regardless, retain/release has become only 4% of your runtime in this benchmark, and I believe there isn't any further win in ByteBuffer in your current benchmark, so I'm going to close this issue for now.

@Lukasa Lukasa closed this as completed Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants