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

Reduce the memory footprint of ManagedWebSocket #52022

Merged
merged 1 commit into from
May 4, 2021

Conversation

zlatanov
Copy link
Contributor

@zlatanov zlatanov commented Apr 28, 2021

While working on the websocket compression I noticed two things but decided to leave them after the feature was completed:

  1. There was a static readonly instance of RandomNumberGenerator, which could be replaced with RandomNumberGenerator.Fill.
  2. The ManagedWebSocket class was creating CancellationTokenSource but never really used the cancellation part. Since continuations from CancellationTokenSource.Cancel are executed synchronously, the cancellation source could be replaced with a method.

This reduced the memory footprint of a websocket without compression enabled from 752 bytes to 552 bytes.

//cc @CarnaViire @scalablecory

…ill instead of custom instance of RandomNumberGenerator.
@ghost
Copy link

ghost commented Apr 28, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

While working on the websocket compression I noticed two things but decided to leave them after the feature was completed:

  1. There was a static readonly instance of RandomNumberGenerator, which could be replaced with RandomNumberGenerator.Fill.
  2. The ManagedWebSocket class was creating CancellationTokenSource but never really used the cancellation part. Since continuations from CancellationTokenSource.Cancel are executed synchronously, the cancellation source could be replaced with a method.

This reduced the memory footprint of a websocket without compression state from 752 bytes to 552 bytes.

//cc @CarnaViire @scalablecory

Author: zlatanov
Assignees: -
Labels:

area-System.Net

Milestone: -

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I see no reason why we'd want to use a CTS here

@CarnaViire
Copy link
Member

FYI: CI failures are ongoing infra issue. I will rerun checks after it's fixed

@zlatanov
Copy link
Contributor Author

zlatanov commented May 4, 2021

@CarnaViire the failing tests seem unrelated. Let me know if anything else is needed.

@CarnaViire
Copy link
Member

@zlatanov sorry for delaying on the rerun 😔 I've triggered it now
As I understood it, because of the CI issue on Thursday-Friday, it didn't run the whole Browser wasm testing step, so I believe it's better to wait for it to complete. Hopefully it'll be green so I'll merge it afterwards 🙂

@CarnaViire CarnaViire merged commit 576be0d into dotnet:main May 4, 2021
@zlatanov zlatanov deleted the websocket-memory-footprint branch May 11, 2021 16:39
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants