Change System.Net.WebSockets contract for WebSocketReceiveResult

[benaadams]

Follow up from "Resuable WebSocketReceiveResult" dotnet/corefx#4520

At the moment is WebSocketReceiveResult is a read only POCO type that as a class must be garbage collected on every single socket message; probably gen0; but still bad for high speed websockets.

I can see two approaches to address this:

1. Change the setters to protected, which opens up a possibility for types derived from System.Net.WebSockets.WebSocket to reuse the result object using a derived WebSocketReceiveResult type.
2. Change type from class to struct.

Either approach halves the number of allocations, second is safer but allocates more memory. (Though ValueTask returns could change this)

[benaadams]

cc: @davidsh

[davidfowl]

I always wondered why it wasn't a struct. Does anyone derive from this type?

[benaadams]

While breaking contracts might be useful if it was a struct and ReceiveAsync returned a ValueTask<WebSocketReceiveResult> instead...

[ghost]

(perhaps a rookie question)
How would changing class to struct makes data reusable?

[davidsh]

cc: @CIPop @pgavlin @SidharthNabar @bartonjs @terrajobst

[benaadams]

@jasonwilliams200OK it doesn't; it means the GC doesn't allocate an object handle for it which it then needs to check and clean up.

[CIPop]

@benaadams Thanks for your input!
Could you share any perf numbers observed in your real-world scenario when you make this change?

The reasons we can't change the contract is portability between Desktop and CoreCLR. We would consider adding another overload if the perf increase in a real-world scenario justifies it.

At the same time, please note that we are not yet done with the WebSockets implementation and porting all tests. To avoid destabilizing the implementation, this work will be postponed until we've implemented and tested the existing contract.

I always wondered why it wasn't a struct. Does anyone derive from this type?

@davidfowl Nothing derives or should be deriving from this type. I don't know/understand the original design decision either.

How would changing class to struct makes data reusable?

@jasonwilliams200OK See https://msdn.microsoft.com/en-us/library/ms173109.aspx for clarification. The short answer is that struct is a value-type and will not be managed by GC.

[ghost]

@benaadams, @CIPop thanks for the answers and the link. The answers at http://stackoverflow.com/q/2146434 by @jskeet and @ericlippert further clarified on value-type datastructures' instances are GC-agnostic and instance they are virtually popped out by reverting the CPU stack pointer back when IP goes out of scope (on function return for example).

[benaadams]

@jasonwilliams200OK unless they are instance members of a class; but the GC still doesn't pay attention to them - same behaviour as adding an int to a class though probably larger.

[benaadams]

@CIPop this is a perfview of our current allocators and WebSocketReceiveResult is about 17th on the list

[CIPop]

/cc: @himadrisarkar

Thanks @benaadams!

[benaadams]

After some optimizations and upgrading to aspnet Websocket 0.2.0 which shares corefx's impl

WebSocketReceiveResult and Task<WebSocketReceiveResult> are items 3 and 5 on our top allocated items (on non-ssl):

[stephentoub]

@benaadams, I realize this is an aside from the main purpose of your post, but I assume that these moving up on the list speaks well to the rest of the implementation, in that other things higher on the list have been removed?

[benaadams]

@stephentoub yes it does, am using the 1.1.0 dotnet-core myget feed 👍

There's ReceiveAsyncPrivate and EnureBufferContainsAsync which are new; but they may just be actual asyncness

[stephentoub]

Are those SemaphoreSlim.TaskNodes coming from the websockets implementation? That would imply a lot of concurrent receives or sends (or a bug).

[benaadams]

No, is the signalling mechanism for data/no data, looking to do a custom awaiter as its single producer (dedicated thread), single consumer (thread-pool).

[benaadams]

@stephentoub there are other allocations in websockets (byte[] etc); but doesn't seem to anything too major by comparison.

[davidfowl]

FWIW, the ASP.NET team are looking at defining an alternate contract for websockets that doesn't require users to allocate a buffer or task per read. Still very early though.

/cc @anurse

[CIPop]

The reasons we can't change the contract is portability between Desktop and CoreCLR. We would consider adding another overload if the perf increase in a real-world scenario justifies it.

@benaadams I think we can't change the existing APIs. WebSocketReceiveResult is part of netstandard.

I still think this would be a good idea but it'll require adding new APIs in System.Net.WebSockets that return the structs instead. While perf will be improved, the API surface will not.

@terrajobst @weshaggard @karelz would having both APIs be allowed? (We would need to add StructWebSocketReceiveResult as a new struct type to the public API as well.)

      public abstract System.Threading.Tasks.Task<System.Net.WebSockets.WebSocketReceiveResult> ReceiveAsync(System.ArraySegment<byte> buffer, System.Threading.CancellationToken cancellationToken);
// and something on the lines of:
      public abstract System.Threading.Tasks.Task<StructWebSocketReceiveResult> ReceiveAsync(System.ArraySegment<byte> buffer, System.Threading.CancellationToken cancellationToken);       

[karelz]

You cannot overload method on return type, but in principle if there is non-trivial perf benefit, I'd be ok adding new API.
@davidfowl is it based on Channels/Pipelines (or whatever the current name is)? In that case we might want to wait for that API, instead of trying to patch up performance of current stack.

@geoffkizer @benaadams any opinions?

[benaadams]

It would have to be virtual and wrap the current method in the base as there are multiple implementations of it already.

I think its worth addressing, as it is the standard WebSocket base that everything implements. Even Pipelines would likely have to provide a derivation of it in some way (even if just a wrapper transform).

If adding a new method; I wouldn't just stop at the struct return, but go all the way... Since is return max size of message; rather than max size of buffer you can have many messages in the buffer so the call has the opportunity to sync complete more frequently.

So either ValueTask with a struct return

public virtual ValueTask<WebSocketMessageReceiveResult> 
    ReceiveAsync(
        ArraySegment<byte> buffer, 
        CancellationToken cancellationToken);

Alternatively, don't change the Receive function at all; since it is already a class; and since only one Receive can happen at once (else I think it throws); you could add an extra property to the WebSocket

public bool AllowReceiveResultReuse { get; set; }

Then derived classes could optionally reuse the same instance and also the same cached task with that instance and just change the values?

[benaadams]

May be resolved by https://github.com/dotnet/corefx/issues/21281

public virtual ValueTask<ValueWebSocketReceiveResult> 
    ReceiveAsync(Buffer<byte> buffer, CancellationToken cancellationToken = default(CancellationToken));

[stephentoub]

Replaced by https://github.com/dotnet/corefx/issues/22610. If anyone feels otherwise, please feel free to comment/re-open.