Zaid-Ajaj / Fable.Remoting

Type-safe communication layer (RPC-style) for F# featuring Fable and .NET Apps
https://zaid-ajaj.github.io/Fable.Remoting/
MIT License
272 stars 54 forks source link

Do not instantiate API record until it is necessary #193

Closed kerams closed 3 years ago

kerams commented 3 years ago

Implements the second point in https://github.com/Zaid-Ajaj/Fable.Remoting/pull/177 all the way.

It's just a tiny optimization, so there's no need to release it on its own.

Zaid-Ajaj commented 3 years ago

Hi there @kerams,

Can you please tell where the optimization is done here? The PR only delays seems building of the implementation for later in the pipeline but it is already delayed since it depends on the incoming request and the execution of the awaitable task 🤔 am I missing something, maybe?

kerams commented 3 years ago

The record is created before the current handler is confirmed to be a match. props below contains an implementation even though it ends up unused if the record does not contain the function we're looking for.

https://github.com/Zaid-Ajaj/Fable.Remoting/blob/3ebe8eee62ba981b7ea43e71ce971605905ea1a6/Fable.Remoting.Server/Proxy.fs#L154-L157

This is especially noticeable when you have lots of functions split into multiple separate APIs/records

 .UseGiraffe (choose [ api1Handler; api2Handler; api3Handler, api4Handler; api5Handler; api6Handler ])

An incoming request for a function in the last handler currently causes the instantiation of implementations for APIs 1 through 5.

Zaid-Ajaj commented 3 years ago

Thanks for the explanation, that makes sense. Though I had in mind that those instantiations are really cheap to begin with but it doesn't hurt to push for more optimizations 🚀

kerams commented 3 years ago

Sure, though they can potentially instantiate more expensive dependencies of their own. If using the approach described in the docs

let createTodoApiFromContext (httpContext: HttpContext) : ITodoApi = 
    let todoStore = httpContext.GetService<ITodoStore>()
    createTodoApi todoStore

Imagine you were also pulling something from the database there because you know it's needed in every function of the record. Very unlikely but plausible.

Zaid-Ajaj commented 3 years ago

Yeah it is good not to assume or expect things from user code (like always having a cheap factory function)

Zaid-Ajaj commented 3 years ago

Published new versions of Server (base) and adapters Suave, Giraffe and AspNetCore which include the improvements in this PR 🚀