Improving SSE Performance

[mkarg]

SSE, in contrast to "usual" polling-style REST APIs, typically implies a comparably higher amount of messages sent to a comparably higher amount of peers, as a lot of events is sent to a lot of observers each time a resource is modified.

To reduce server load, it makes sense to make the event sending part as lean as possible.

Due to that reasoning I propose the adoption of the attached changes:

• Make the actual critical path (i. e. the inner per-character-loop needed to attach the DATA_LEAD prefix) as fast as possible, making it at most likely for inlining, even if it might make the code three lines longer and slightly harder to read.
• Don't use lookups where static information can be provided (here: UTF-8 charset) - This change is independent of SSE, but I just came across it when looking at the sole changed code file.

NB: The outcome of this PR is even more correct than the original code, as a trailing \n as the last character produced by the "inner" MBW previously lead to a "dangling" (i. e. otherwise empty) DATA_LEAD prefix!

[jansupol]

This indeed is a bit faster, thanks

[jansupol]

On the other hand, there is a difference in the code, if "\n" is the last symbol written, the DATA_LEAD is not added. (I see your comment about this)

[mkarg]

On the other hand, there is a difference in the code, if "\n" is the last symbol written, the DATA_LEAD is not added. (I see your comment about this)

Correct, this is a bug fix. The original code wrote an excess data line: past that case, which made no sense IMHO.

[jansupol]

Suppose the following:

        @GET
        @Path("eventStream")
        @Produces(MediaType.SERVER_SENT_EVENTS)
        public void send(@Context SseEventSink eventSink, @Context Sse sse) {
            try (SseEventSink sink = eventSink) {
                eventSink.send(sse.newEvent("event1"));
                eventSink.send(sse.newEvent("\r\n"));
            }
        }

With the current code, the last InboundEvent contains the new line. With this PR, the InboundEvent on the client does not receive the new line. That looks like a regression.

[jansupol]

Also, sse.newEvent("\n") duplicates an empty event...

[mkarg]

With the current code, the last InboundEvent contains the new line. With this PR, the InboundEvent on the client does not receive the new line. That looks like a regression.

Yes and no, it depends how you define the word "regression". 😉

Actually the old code was incorrect. It accepted \r as event text within a data: line, which is forbidden by the SSE spec: \r, just like \n MUST NOT be found in the middle of a data: line, as it is not part of the valid set of characters:

any-char      = %x0000-0009 / %x000B-000C / %x000E-10FFFF
                ; a [scalar value](https://infra.spec.whatwg.org/#scalar-value) other than U+000A LINE FEED (LF) or U+000D CARRIAGE RETURN (CR)

Even worse, the exact combination \r\n is to be treated as one single EOL indicator, but not as user text \r plus EOL. Both is wrong in old and new code, but wrong in different ways. Hence, this PR does not really make to code worse than it is, it just makes it "differently wrong" -- plus running faster. ☺️ Hence, it is irrelevant whether the old and new code have different results. Instead, we should concentrate to fix it in a away that is correct -- even if that means that it behaves differently than the original code.

Having said that, I will file another PR which first corrects the wrong handling of \r and \r\n (even if it breaks backwards compatibility and runs slower), and come back later to this SSE Performance PR.

[mkarg]

any-char      = %x0000-0009 / %x000B-000C / %x000E-10FFFF
                ; a [scalar value](https://infra.spec.whatwg.org/#scalar-value) other than U+000A LINE FEED (LF) or U+000D CARRIAGE RETURN (CR)

Minor addition, forgot to mention earlier: As a result of this definition in the SSE Spec, users MUST NOT expect to receive the exact same line breaking bytes on the receiver side that they put into place in the sending side. The only guarantee they have is that there will be ONE line break on the receiving end if there was either \n, \r or \r\n on the sending side. In particular this means, if the sending side wanted to send two blank lines using the text "\r\n" they MUST receive just one blank line, actually! ☝️

[jansupol]

I would not change 2.x, and update just 3.1.

[mkarg]

I would not change 2.x, and update just 3.1.

Okay so I will target the bug fix PR for the 3.1 branch only.