search

kitten / pessimism

A fast HAMT Map intended for KV caching and optimistic updates
MIT License
15 stars 3 forks source link

Should we remove use of `unsafe` methods? #1

Open parkerziegler opened 5 years ago

parkerziegler commented 5 years ago

👋 Hey @kitten, happy Saturday! Got some time to read through pessimism and it's really fun to learn about! My one question is, I notice we have quite a lot of use of unsafe_get methods on strings and arrays, mostly, I'm assuming, to not have to deal with option types. Was this more to get this working quickly or because it's not really an eventuality we need to deal with? Admittedly, I'm still pretty foggy on how HAMTs work (the Wikipedia is a bit over my head 😅), so was curious to get some clarification here.

I'm also speculating on whether or not we could use Reason's native array for some of the operations in an effort to remove Js.Array entirely – the benefit I see there is that it can then be used in native Reason / OCaml for that glorious day when someone writes a native GraphQL client for Reason 😂. In any case, just curious and interested to hear your thoughts. If you'd be open to removing the unsafe_ methods I'd love to make a PR for it and get my feet wet!

kitten commented 5 years ago

Hey Parker 👋

Sorry for the late reply. I haven’t made this native-compatible yet because I was optimising some operations for JS/v8. Since I do have the Rebel code files that have some shared Array operations porting this shouldn’t be too hard.

The problem I see with porting this is that resizable arrays have different performance characteristics than fixed size arrays. So it may turn out that native needs to be implemented differently. Hence I held off from implementing this already.

Resizing arrays isn’t possible with OCaml’s arrays, since they’re fixed blocks of memory. So either we have to change them to allow for preinitialised memory (which will worden memory usage on native) or come up with something else