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

Use recyclable memory streams #280

Merged
merged 3 commits into from
Dec 6, 2021
Merged

Use recyclable memory streams #280

merged 3 commits into from
Dec 6, 2021

Conversation

kerams
Copy link
Collaborator

@kerams kerams commented Nov 13, 2021

Just some more perf optimizations by reusing temporary output streams (we avoid reallocating an output buffer for every response) thanks to Microsoft.IO.RecyclableMemoryStream. Giraffe already uses this internally, so for the majority of users this won't be a new dependency.

I've also added Remoting.withRecyclableMemoryStreamManager

/// Enables you to provide your own instance of a recyclable memory stream manager
let withRecyclableMemoryStreamManager rmsManager options =
    { options with RmsManager = Some rmsManager }

This aligns with giraffe-fsharp/Giraffe#494, so that you could use recyclable memory streams on your own, in Giraffe and in Remoting, and all 3 would share the same memory stream pool (which has a couple of settings you can tune) if you wish.

@Zaid-Ajaj Zaid-Ajaj merged commit 4e7bd37 into master Dec 6, 2021
@kerams kerams deleted the rms branch December 6, 2021 13:27
@Zaid-Ajaj
Copy link
Owner

Awesome work as always @kerams ❤️

Before I publish this, I was actually checking out the implementation in Giraffe and it seems that Giraffe now registers a default RMS. Is it an idea for Remoting to actually try grab that registered RMS and use internally by default? i.e. use what's available:

  • See if RemotingOptions have a RMS
  • if not available, try resolve it from the HttpContext,
  • if not available, use the default one available from Remoting.Server?

Also, what do you think about Remoting.withRecyclableMemoryStreamManager being of type HttpContext -> RecyclableMemoryStreamManager?

Finally, maybe a small rename withRecyclableMemoryStreamManager -> withStreamManager?

@kerams
Copy link
Collaborator Author

kerams commented Dec 6, 2021

There's one problem. buildFromImplementation creates a proxy with the stream manager before there are any requests and HTTP contexts. In other words, at that point it's not (easily?) possible to look up services registered in the DI container.

Also, what do you think about Remoting.withRecyclableMemoryStreamManager being of type HttpContext -> RecyclableMemoryStreamManager?

I don't quite know what you want to achieve with that :).

withRecyclableMemoryStreamManager -> withStreamManager?

Sure.

@Zaid-Ajaj
Copy link
Owner

Also, what do you think about Remoting.withRecyclableMemoryStreamManager being of type HttpContext -> RecyclableMemoryStreamManager?

I don't quite know what you want to achieve with that :)

I want to make it easy to require the already injected RMS from Giraffe:

|> Remoting.withStreamManager (fun (context: HttpContext) -> 
    let rms = context.GetService<RecyclableMemoryStreamManager>()
    Some rms
)

Without having to define one on your own. Does this make sense?

Should a default implementation already do that so that users of Giraffe & Remoting get a shared RMS without doing extra work?

@kerams
Copy link
Collaborator Author

kerams commented Dec 6, 2021

That would be unusable in Suave as it does not have any built-in DI as far as I know. We could have both your function and mine to configure the RMSM, but it would be a bit silly to complicate things so much for something almost no one will really bother to customize.

I think what we have right now is the best solution since it's webserver/DI-agnostic. No injection through any containers, just direct plugging in via RemotingOptions.

Zaid-Ajaj added a commit that referenced this pull request Dec 7, 2021
@Zaid-Ajaj
Copy link
Owner

I think what we have right now is the best solution since it's webserver/DI-agnostic

Then I'll leave it as is, I trust your judgement 💯 published a new batch of packages that includes the feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants