Dynamic dispatch for storage?

datrs / hypercore

Secure, distributed, append-only log

https://docs.rs/hypercore

Apache License 2.0

333 stars 37 forks source link

Dynamic dispatch for storage? #109

Closed Frando closed 4 months ago

Frando commented 4 years ago

Hi, I've been thinking about whether it would make sense to remove the generic Storage<T> field on the Feed struct and instead have a Box<dyn DynStorage> object on the Feed. DynStorage would be a trait with all public methods of Storage<T>. Thus, Feed<T> would just be Feed.

Benefits:

More ergonomic to work with in code that uses Feed. This becomes more apparent on the branch that removes failure (#102) where the generic type argument becomes really long and has to be carried through all code that uses Feed.
Allows to combine both in-memory and disk-backed Feeds in a container. Currently, that wouldn't be possible.

Drawbacks:

Cost of dynamic dispatch

I would guess that compiler optimizations might oftenly reduce the cost of dynamic dispatch if you'd only use one implementation in your code paths, but I'm not sure about it.

Opinions?

Frando commented 4 years ago

@yoshuawuyts do you have an opinion on this?

Frando commented 4 years ago

We could even have a dyn Storage for each storage (keypair, data, bitfield etc). The Node impl allows to have different storages and e.g. corestore uses this to have a different key storage for feeds it manages. I'm not sure what the cost of dynamic dispatch actually is though.

yoshuawuyts commented 4 years ago

@Frando big fan of erasing types in cases like these. Think this is a reasonable direction to explore.

bltavares commented 4 years ago

Great!

@Frando if you haven't already started, I could take a look. I am planning to do some love coding this Sunday and I could box the types

Em qua, 13 de mai de 2020 08:23, Yoshua Wuyts notifications@github.com escreveu:

@Frando https://github.com/Frando big fan of erasing types in cases like these. Think this is a reasonable direction to explore.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/datrs/hypercore/issues/109#issuecomment-627918652, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2XIVHM5BZC5FDQRPB4ZDRRJ7JTANCNFSM4MD3MMFA .

Frando commented 4 years ago

@bltavares very cool. I did start a bit on this some time but didn't publish yet. I opened #113 with what I have. It's unfinished. Please feel very free to continue there and just push to the branch dircectly, and/or do a new clean start. I ran into an issue wrt to boxing the individual RandomAccess instances, it's explained in a comment on #113. If you have an idea on how to solve that that would be amazing. Otherwise I think we can also continue with the approach in #113.