microsoft / FASTER

Fast persistent recoverable log and key-value store + cache, in C# and C++.
https://aka.ms/FASTER
MIT License
6.29k stars 563 forks source link

Boxing interface usage is allocating unnecessary heap data #907

Open mgravell opened 5 months ago

mgravell commented 5 months ago

scenario: using the async API but fully sync in reality after checking completion; each call issues an InternalFasterSession boxed on the heap;

You can see this by running a memory profiler on this branch (branch: faster-boxing; project: /test/Benchmarks) in DEBUG mode (release mode runs BDN; you don't want that). The test runs 25,000 operations in a loop; the only heap alloc is:

Type Allocations | - FASTER.core.ClientSession<FASTER.core.SpanByte, FASTER.core.SpanByte, Input, Output, FASTER.core.Empty, CacheFunctions>.InternalFasterSession 25,000

Since InternalFasterSession is a struct, this is unlikely to be intentional.

This is almost certainly where InternalFasterSession is being passed as a IFasterSession<Key, Value, Input, Output, Context> - for example DoSlowOperation, AsyncOperationInternal, etc, which will force it to be boxed. To avoid boxing, the type must be either:

but: any time it is assigned / passed as IFasterSession<...>: it will box, i.e. allocate

badrishc commented 5 months ago

Your findng is accurate, but the reason we have not addressed it is because there is a standard pattern we use to avoid this:

var status = sync-call
if (status.IsPending)
   (await or sync-wait) complete-pending

An example for Read along these lines is in Garnet here: https://github.com/microsoft/garnet/blob/main/libs/server/Storage/Session/MainStore/MainStoreOps.cs#L18

mgravell commented 5 months ago

To confirm, then: using the synchronous API, checking IsPending, and in the pending scenario using asynchronous completion: is equivalent (functionally) to using the asynchronous API in the first instance? (give or take having to wire up cancellation etc, which I can probably do)

On Sun, 31 Mar 2024, 21:11 Badrish Chandramouli, @.***> wrote:

Your findng is accurate, but the reason we have not addressed it is because there is a standard pattern we use to avoid this:

var status = sync-call if (status.IsPending) (await or sync-wait) complete-pending

An example for Read along these lines is in Garnet here: https://github.com/microsoft/garnet/blob/main/libs/server/Storage/Session/MainStore/MainStoreOps.cs#L18

— Reply to this email directly, view it on GitHub https://github.com/microsoft/FASTER/issues/907#issuecomment-2028893336 or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMF3ZI6MRK7L7B33CBDY3BUW7BFKMF2HI4TJMJ2XIZLTSOBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLLDTOVRGUZLDORPXI6LQMWWES43TOVSUG33NNVSW45FGORXXA2LDOOJIFJDUPFYGLKTSMVYG643JORXXE6NFOZQWY5LFVEYTIMZSGUZTAMRQQKSHI6LQMWSWS43TOVS2K5TBNR2WLKRSGIYTIMBSGQ2DKMVHORZGSZ3HMVZKMY3SMVQXIZI . You are receiving this email because you authored the thread.

Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub .

mgravell commented 5 months ago

@badrishc I've spiked a switchover to the synchronous API exclusively, with any mention of async only in the (rare) IsPending scenario; I'll run the numbers when my machine is working