Performance improvements to queue(s) management in Webserver

[spericas]

The Webserver keeps track of a queue of queues to release buffers back to Netty. Every new request that comes in creates a queue to track the Netty buffers. The cost of removing one of these queues when no longer needed is O(N) where N is the number of active connections. When N is large (e.g. 16K) this housekeeping operation can be costly.

This PR uses a different approach that avoids the O(N) removal operation. It uses phantom references and lets the GC gather the queues when the associated publisher becomes ready for collection. A clearQueues method is called periodically to clean up the queue of queues. Thanks to @olotenko.

[ljnelson]

Why lazySet?

[olotenko]

then it is a normal store (no expensive full barrier)

(think mov mm, r vs lock: mov mm, r)

in this case it is sufficient, because IndirectReference is published safely (becomes reachable from other threads only as a result of some operation that has full barriers).

[olotenko]

Actually, I am not sure that the atomicity of the reference is even required here.

All accesses are meant to be single-threaded in the intended use. It is atomic only for the purpose of potential other uses.

[spericas]

Maybe not needed, but seems like the safer option.

[olotenko]

I understand all apart from the removal of ChannelHandlerContext ctx from HttpRequestScopedPublisher.

Also, I would do the handling of failPublisher for 4xx errors as a separate commit, but separating that is not essential.

As far as the reference queue handling is concerned, it is good to ship.

[spericas]

I understand all apart from the removal of ChannelHandlerContext ctx from HttpRequestScopedPublisher.

That context is no longer used in the publisher.

Also, I would do the handling of failPublisher for 4xx errors as a separate commit, but separating that is not essential.

As far as the reference queue handling is concerned, it is good to ship.

Cool, thx.

[ljnelson]

Just being careful: is Object the type argument you want to pass here?

[olotenko]

Correct. That type-parameter is silly. (Reads: I don't get it) It is meant to represent the type of element returned by References enqueued in ReferenceQueue. But by the time a reference is enqueued the element is no longer reachable, as determined by GC. So...

What would make more sense as a type parameter for the ReferenceQueue, is the class of Reference that is going to be returned by poll.

[ljnelson]

Well, I don't know about silly. Would ? super Reference<?> or something like that work better here?

[olotenko]

No.

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/ReferenceQueue.html#poll() - ReferenceQueue<T>.poll() returns Reference<? extends T>. I am not interested in what T is, because that value is no longer accessible. I am only interested in the type of Reference. If it is something that has a method to release whatever resource it is associated with (ie instanceof IndirectReference in this case), I am going to call it - end of story.

I'd rather have a guarantee that I will not encounter other type of Reference here, so that I didn't have to type-cast. :)

I'd prefer:

public class ReferenceQueue<T extends Reference> {
...
   public T poll() {...}
}

Then I'd be able to declare ReferenceQueue<IndirectReference<?, ReferenceQueue<IndirectReference<?, DataChunk>>>> queues and get IndirectReference<ReferenceQueue<IndirectReference<?, DataChunk>>> r = queues.poll() and ReferenceQueue<IndirectReference<?, DataChunk>> rq = r.acquire(), and then IndirectReference<?, DataChunk> rr = rq.poll() and even DataChunk dc = rr.acquire() and dc.release() (gosh, this is actually what this is for).

[olotenko]

Think of it this way: IndirectReference is about two things: some owner of type T that actually does not matter, and a resource R that is temporarily handed off to the owner of type T. The owner has the obligation to always terminate in a predictable path where it hands off the resource R back to some resource pool.

IndirectReference then is a safety net catching the cases when the owner T fails to fulfil the obligation (Entscheidungsproblem and a Turing-complete language), and dies before returning resource R. In this case IndirectReference ends up in the ReferenceQueue, and the queue processing routine is able to return resource R back to the pool. Note only the owner of type T is referenced phantomly. The resource R remains strongly reachable, at least through IndirectReference - unless, of course, the owner does fulfil the obligation, and returns the resource, first reclaiming it through IndirectReference.acquire().

[olotenko]

Having explained all this, I think more work is needed.

The focus has been to reduce the cost of maintaining resources for well-terminating responses. But need to consider what happens to strong references when the channel is closed (channelInactive fired) and there are unfinished responses.

[ljnelson]

Just being careful: lazySet is designed for very specialized use cases. Are you sure set isn't the right choice here?

[olotenko]

Yes, this is one case where lazySet makes sense.

We only have a lock that can either be taken immediately, or the contender goes away:
https://github.com/oracle/helidon/pull/2704/files/06c459f90972ba6a14a500bfaea7cd4b91beb3cb#diff-a0cac19f24ae85fbc99afd8c0b4e8375d3c036a0ad949c68befe56fb7346461dR100 - that yellow line

In this case we only need to ensure the get() and compareAndSet on that line synchronize-with this lazySet, which they do.

We don't need to "publish" any other changes, as queue poll and removeIf are thread-safe in their own right.

In this case you can even go as weak as VarHandle.setOpaque.