Async publishing and API proposals #39

mtmk · 2024-02-27T03:27:02Z

See updated readme here https://github.com/nats-io/nats.swift/tree/fix-async-pub

While updating the examples I made some changes to the API to make is a little more dev friendly:

made publish async
publish subject is named as to: to match subscribe()
changed how options are accepted moving away from builder (this is experimental)
added easier to use subscripts for headers
updated readme and examples

piotrpio

Some great additions. A few questions regarding performance and the new NatsConnection class.

piotrpio · 2024-02-27T11:42:01Z

README.md

+You can also customize your connection by specifying additional options:
+
+```swift
+let options = NatsOptions(


To be honest, for more advanced options setup I personally prefer builder to a large public constructor. For example, builder does not force you to pass arguments in specific order and simplifies options validation if necessary. That said, I am not strongly against this, this is just my preferred approach.

We had this approach before, and it's also easy in Swift to run into conflicts when passing different things of the same type - especially around Auth.

I was assuming you can pass the params in any order same as in c#. we should double check that.

validation is a good point. it can be done in constructor and/or in the connection class in this case for example.

I see, we should cater for habits which is important, and we must if that's the case here. The idea is that gradually abandoning these patterns (builder in this case) as the languages becoming more expressive in their syntax which in turn invalidates the initial reason to have patterns in the first place.

/rant: whenever I see a .build() at the end of a call chain my eyes start burning 😅 /

The order thing surprised me a lot too. It looks so conuter intuitive and unnecesary if you have named parameters.

piotrpio · 2024-02-27T11:46:05Z

README.md

-| dropped      | The clients droped a message. Mostly because of queue length to short. |
-| reconnecting | The client reconencts to the server, (Because of a called reconnect()).|
-| informed     | The server sends his information data successfully to the client.      |
+| event        | description                                                             |


This list of events is from the old client, but I can update it in a separate PR

I would remove the list entirely. It's easy enough to check the code/docs.

didn't know that. we should probably ditch that table in that case.

piotrpio · 2024-02-27T11:51:02Z

Sources/NatsSwift/NatsConnection.swift

            }
        }
    }

-    func write(operation: ClientOp) throws {
+    func write(operation: ClientOp) async throws {


Out of curiosity - did you compare the benchmark times before and after making publish async? I remember trying it at first and for whatever reason, the result was dramatically worse when using async

it's about 3x slower. The problem is though just writing without flushing and ignoring concurrency isn't the right use of nio api. For example in this case I've noticed the benchmark can consume literally gigabytes of memory whereas with proper async it's at a few megs. we need to find proper ways of addressing the performance issue.

Also to note, we had similar issues with c#. not using async was producing better results. in the end the problem wasn't at all to do with async/await but managing the buffers to make the socket write calls in an optimal frequency.

For me it's 10x worse performance.
However I disagree that to have proper NIO usage you have to flush after every message. You can flush whenever you feel to, and NIO should flush if buffer was filled faster. The if for flushing I added was trying to check if bufer is busy enough that it's not worth flushing and it was a nice balance between throughput and latency.

On the side of allocations - keep in mind that more 3x/10x more throughput can also bump as much the allocations.
However, I would hope that you can set the max buffer size of NIO internal buffer....

piotrpio · 2024-02-27T12:04:28Z

Sources/NatsSwift/NatsClient/NatsClient.swift

+// Experimetal class to make applying options a bit more natural.
+// This is also very close to how C# client is designed.
+// If accepted we can refactor `Client` and `ClientOptions` types.
+public class NatsConnection {


As mentioned before, I am not completely sold on this approach, @Jarema what do you think?

btw just to be clear I'm not suggesting we leave this as it is. please only consider the public api i.e. options construction/building/maybe naming. we can refactor internals later. Also it would be nice to see how other libraries are solving the options issue. be nice to have a little survey of that.

btw (this is somewhat out of context) in terms of public API for c# we made the decision to prefix all public types with Nats* which I think helps when application code is using several libraries and you want to avoid clashes both for readability and avoiding namespace manipulation/aliasing/etc. overhead. This is especially helpful with overused generic names like client, stream or options (I know we don't have stream at the moment but I'm thinking future use as well).

piotrpio · 2024-02-27T12:06:19Z

Sources/NatsSwift/NatsHeaders.swift

@@ -108,4 +112,68 @@ extension HeaderMap {
            }
        }
    }
+
+    public subscript(name: String) -> String? {


That and the iterator are great additions! 👍

thanks! sorry for the long responses above, hope it makes sense 😅

Jarema

Thank you for hard work on the Readme!

I think we should split other changes into separate discussions / PRs thoufh.

Jarema · 2024-02-27T14:23:24Z

README.md

-| dropped      | The clients droped a message. Mostly because of queue length to short. |
-| reconnecting | The client reconencts to the server, (Because of a called reconnect()).|
-| informed     | The server sends his information data successfully to the client.      |
+| event        | description                                                             |


I would remove the list entirely. It's easy enough to check the code/docs.

Jarema · 2024-02-27T14:25:43Z

README.md

+You can also customize your connection by specifying additional options:
+
+```swift
+let options = NatsOptions(


We had this approach before, and it's also easy in Swift to run into conflicts when passing different things of the same type - especially around Auth.

Jarema · 2024-02-27T18:23:00Z

Sources/NatsSwift/NatsClient/NatsClient.swift

@@ -79,20 +79,12 @@ extension Client {

    public func publish(
        _ payload: Data, subject: String, reply: String? = nil, headers: HeaderMap? = nil
-    ) throws {
+    ) async throws {


This, at least on my system, cuts the performance by 10x.
I would avoid adding the async flag unless we find a way to make it performant.

Jarema · 2024-02-27T21:08:31Z

README.md

+You can also customize your connection by specifying additional options:
+
+```swift
+let options = NatsOptions(


The order thing surprised me a lot too. It looks so conuter intuitive and unnecesary if you have named parameters.

Jarema · 2024-02-27T21:11:28Z

Sources/NatsSwift/NatsConnection.swift

            }
        }
    }

-    func write(operation: ClientOp) throws {
+    func write(operation: ClientOp) async throws {


For me it's 10x worse performance.
However I disagree that to have proper NIO usage you have to flush after every message. You can flush whenever you feel to, and NIO should flush if buffer was filled faster. The if for flushing I added was trying to check if bufer is busy enough that it's not worth flushing and it was a nice balance between throughput and latency.

On the side of allocations - keep in mind that more 3x/10x more throughput can also bump as much the allocations.
However, I would hope that you can set the max buffer size of NIO internal buffer....

mtmk added 2 commits February 27, 2024 00:59

Publish async fix

3146904

Examples and headers tweak

50ddb17

mtmk changed the title ~~Fix async pub~~ Docs, examples and async pub Feb 27, 2024

readme and client update

6e43c4c

mtmk requested review from Jarema and piotrpio February 27, 2024 07:36

mtmk marked this pull request as ready for review February 27, 2024 07:36

Docs tweaks

a6b30e4

piotrpio reviewed Feb 27, 2024

View reviewed changes

Jarema requested changes Feb 27, 2024

View reviewed changes

mtmk changed the title ~~Docs, examples and async pub~~ Async publishing and API proposals Feb 28, 2024

mtmk closed this Mar 13, 2024

mtmk deleted the fix-async-pub branch March 13, 2024 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async publishing and API proposals #39

Async publishing and API proposals #39

mtmk commented Feb 27, 2024 •

edited

Loading

piotrpio left a comment

piotrpio Feb 27, 2024

Jarema Feb 27, 2024

mtmk Feb 27, 2024

Jarema Feb 27, 2024

piotrpio Feb 27, 2024

Jarema Feb 27, 2024

mtmk Feb 27, 2024

piotrpio Feb 27, 2024

mtmk Feb 27, 2024

Jarema Feb 27, 2024

piotrpio Feb 27, 2024

mtmk Feb 27, 2024

piotrpio Feb 27, 2024

mtmk Feb 27, 2024

Jarema left a comment

Jarema Feb 27, 2024

Jarema Feb 27, 2024

Jarema Feb 27, 2024

Jarema Feb 27, 2024

Jarema Feb 27, 2024

Async publishing and API proposals #39

Async publishing and API proposals #39

Conversation

mtmk commented Feb 27, 2024 • edited Loading

piotrpio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtmk commented Feb 27, 2024 •

edited

Loading