replikativ / hitchhiker-tree

Functional, persistent, off-heap, high performance data structure
Eclipse Public License 1.0
44 stars 19 forks source link

[wip] first pass at refactoring #5

Closed mpenet closed 5 years ago

mpenet commented 5 years ago

As marked on the title it's a work in progress, I just moved things around for now and try to clean up some things like imports. No radical change to the way async or errors are handled just yet for instance.

I am not 100% happy with the file structure but it's getting there.

The tests pass on core/messaging but I didn't test things in detail just yet.

related to #4

Still a lot to do, but I couldn't help it.

mpenet commented 5 years ago

still [wip]

Some observations so far:

whilo commented 5 years ago

I see, yes I remember that async was quite a bit slower when I introduced it. I remember something around a factor 2-4x slower in the insert and delete benchmarks.

mpenet commented 5 years ago

yes I somewhat closed to gap and reached similar numbers as yours. The sync version is equivalent to master.

I also have a branch with promesa instead of core.async, I wanted to give it a try and see if there's a potential difference in performance, but no, it's actually worse and promesa had to be monkey-patched to be usable as completablefuture.

whilo commented 5 years ago

very interesting. hmm, so do you think we have to stick to sync and async version?

mpenet commented 5 years ago

Yes I think it would be wiser.

The pr cleans up quite a bit of the branching for platforms & io so it's already a bit better in terms of readability imho.

Let me know what you think of it, as I took a few liberties along the way. It's also not complete as cljs build is broken right now, but it's prolly just minor adjustments.

whilo commented 5 years ago

I am ok with merging this as is and try to move on to further refactoring and importantly for the rebase towards allowing reverse iterators for datahike.

whilo commented 5 years ago

Does that make sense to you?

mpenet commented 5 years ago

Sure! I am traveling atm so I can't look into it. You very likely need to make sure all tests pass, I think it should be in good shape but I might have missed something with my last modifications.

whilo commented 5 years ago

It seems that flush-tree returns a channel in channel. This breaks the simple konserve tests. I am not sure how this could went wrong. We do not need to implement the reverse iterator for the datahike rebase though, so I would wait until you have time to go through this PR. How is your schedule for the next weeks?

mpenet commented 5 years ago

Odd. I will have a look early next week, it's probably something quite simple.

whilo commented 5 years ago

:+1: Yes, I think so as well. I just haven't had enough time to investigate it yet.

mpenet commented 5 years ago

@whilo It should be fixed now