mgravell / FASTERCache

IDistributedCache implementation on FASTER
MIT License
30 stars 2 forks source link

Use Garnet's storage layer as in-process library, instead of FASTER #10

Closed badrishc closed 5 months ago

badrishc commented 5 months ago

This PR replaces the use of FASTER in this codebase with Garnet in-process (or more specifically Garnet's storage layer library codenamed Tsavorite). Tsavorite is just a fork of FASTER that forms the backend for Garnet. Tsavorite is very similar to FASTER but it is optimized for the cache-store use case, which includes a specialized SpanByteAllocator, the ability to reuse holes in memory, etc. We are also focusing our efforts on the Tsavorite codebase going forward.

As Tsavorite is used by Garnet, it will continue to be better optimized and evolved going forward. It would be easier to have a single codebase that we evolve over time, rather than backporting every update to FASTER.

You will see that the changes to your library are not very deep, in fact some code gets reduced in the process due to SpanByteFunctions<I, O, C> in the base.

It might also be a good idea to rename and move this to be a part of Garnet codebase and NuGet so that people get access to IDistributedCache via that discovery mechanism, and we can make sure we don't make future changes that negatively affect this layer.

mgravell commented 5 months ago

@badrishc amazing, thanks; before I merge - and purely as a formality, you understand - as your first PR here (I think?) and since I don't have a CLA bot here: can I get a "I hereby contribute all changes freely and perpetually in accordance with the stated 'MIT' license" (IANAL, but: that's the best I can do for now!)

If I've understood you correctly: I'm totally fine with any steps towards formalizing this as part of Garnet - ditto any rename/move; that seems an ideal outcome. We need to figure out the specifics, though

NuGet package: that's a little nuanced; there are multiple caching primitives that we will want to support eventually, not just "distributed cache" (that's why I have an "output cache" placeholder, but note that "output cache" requires an additional asp.net framework reference, which is why I haven't added it in the core project); convention for the dotnet-team owned ones is "Caching" - for example "Microsoft.Extensions.Caching.StackExchangeRedis", "Microsoft.Extensions.Caching.SqlServer"; this pattern is also used by some non-dotnet-team owned ones - for example "Microsoft.Extensions.Caching.Cosmos". For consistency, "Microsoft.Extensions.Caching.Garnet" might be the best option, but I'm very open to suggestions. "Microsoft.Garnet.Caching" keeps it more under Garnet's area, but is a break from the common pattern.

Namespace and primary API names: obviously we can mirror those to the package name.

Moving the code under Garnet's direct control; absolutely fine, and if that avoids breaks as Garnet iterates: perfect. My slight concern would be velocity - it is stabilizing, but it has been churning pretty quickly; that's probably mostly over now, so probably not a barrier. In fact, better change control would probably be a good thing now.

mgravell commented 5 months ago

@badrishc one slight reservation; this does change the build graph quite a bit; 5+MB extra dlls, although I'm not too worried about that since anyone who cares will be trimming - but it adds quite a lot of additional packages that we simply don't need, and reduces the runtimes to net6+

I can think of quite a bit of folks (including internal MSFT folks) who would be constrained by this. I suppose there's zero chance of moving Tsavorite to a Microsoft.Garnet.Core NuGet package (with Microsoft.Garnet having a ref to Microsoft.Garnet.Core), perhaps with Microsoft.Garnet.Core also adding back some TFMs? The namespaces and library etc would not change, because Tsavorite is already exposed as Tsavorite via Microsoft.Garnet - it would just be a package called Microsoft.Garnet.Core that bundles Tsavorite.dll (and associated dlls)

(if you're even remotely interested in the idea, I could prepare a PR)

badrishc commented 5 months ago

I hereby contribute all changes freely and perpetually in accordance with the stated MIT license.

badrishc commented 5 months ago

I suppose there's zero chance of moving Tsavorite to ...

This is a great idea and will not be a problem to do. We can create a Microsoft.Garnet.Storage (note the naming) package that Microsoft.Garnet depends upon. That way, your code can simply depend on this NuGet directly.

badrishc commented 5 months ago

Garnet's main nuspec is here: https://github.com/microsoft/garnet/blob/main/Garnet.nuspec Tsavorite lives here: https://github.com/microsoft/garnet/tree/main/libs/storage/Tsavorite

We are not super-familiar with NuGet's latest mechanisms, so above is what we have been using ... Any contributions would be super - would also be happy to also add you to the Garnet team for future contributions to storage.

badrishc commented 5 months ago

If I've understood you correctly: I'm totally fine with any steps towards formalizing this as part of Garnet - ditto any rename/move; that seems an ideal outcome. We need to figure out the specifics, though

NuGet package: that's a little nuanced; there are multiple caching primitives that we will want to support eventually, not just "distributed cache" (that's why I have an "output cache" placeholder, but note that "output cache" requires an additional asp.net framework reference, which is why I haven't added it in the core project); convention for the dotnet-team owned ones is "Caching" - for example "Microsoft.Extensions.Caching.StackExchangeRedis", "Microsoft.Extensions.Caching.SqlServer"; this pattern is also used by some non-dotnet-team owned ones - for example "Microsoft.Extensions.Caching.Cosmos". For consistency, "Microsoft.Extensions.Caching.Garnet" might be the best option, but I'm very open to suggestions. "Microsoft.Garnet.Caching" keeps it more under Garnet's area, but is a break from the common pattern.

Namespace and primary API names: obviously we can mirror those to the package name.

Moving the code under Garnet's direct control; absolutely fine, and if that avoids breaks as Garnet iterates: perfect. My slight concern would be velocity - it is stabilizing, but it has been churning pretty quickly; that's probably mostly over now, so probably not a barrier. In fact, better change control would probably be a good thing now.

Your points make sense. I am fine with any outcome you feel would make sense for both this library, Garnet, and IDistributedCache as a whole. Perhaps for now we can leave it here and once it stabilizes, we can determine how best to "ship" it. :)

badrishc commented 5 months ago

cc @TedHartMS

TedHartMS commented 5 months ago

This is a great idea. We can expose Tsavorite via Microsoft.Garnet.Storage. If I'm understanding correctly, Microsoft.Extensions.Caching. is the standard way to expose IDistributedCache, so we could add a Microsoft.Extensions.Caching.Garnet for discovery of this.

As Badrish indicates, we can look at this when things settle down.

Thanks for your work on this, Marc!