-
-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Immer persist #278
Immer persist #278
Conversation
c96f658
to
80fd4b3
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #278 +/- ##
==========================================
+ Coverage 90.53% 90.54% +0.01%
==========================================
Files 120 121 +1
Lines 12151 12220 +69
==========================================
+ Hits 11001 11065 +64
- Misses 1150 1155 +5 ☔ View full report in Codecov by Sentry. |
818f8d2
to
5344863
Compare
8eefeb9
to
48fa5ad
Compare
6ab55af
to
14e49b2
Compare
8d43497
to
2254d22
Compare
3fc43af
to
023531f
Compare
4ab992b
to
44ffba1
Compare
c7a849e
to
f0d279c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very good, I think this is almost done. Do you think this is ready to get the draft
marker gone?
A couple of polishing remarks, mostly cosmetic stuff, some questions, etc. I'm sorry I'm not doing inline comments, but the diff is so big that the Github UI goes crazy when I try to do so.
-
I see you've introduced also
immer/extra/cereal/...
with some of the serializing code we use in so many other projects. I think this is good, but should we make this complete and include serielizers for all immer types, and get rid of this in the Lager repo: https://github.com/arximboldi/lager/tree/master/lager/extra/cereal -
I suggest splitting the file
immer/extra/persist/cereal/with_pools.hpp
intoimmer/extra/persist/cereal/save.hpp
andimmer/extra/persist/cereal/load.hpp
. These seem to be the main files the library is providing and it is not so clear what the file is about from looking at the current name. -
When looking at the pool internal interface, there is a confusing mix of free functions and methods. For example,
save
is a method ofoutput_pool
but, butadd_to_pool
is not. Maybe they should both be methods? Also what about adding a bit encapsulation? I.e. making the data, and methods that are not meant to be used externally, private? -
It seems
detail/common/pool.hpp
is dead code. Can it be removed? Are there any other bits of code that I'm missing that can be removed? -
It seems some of the implementation files introduce a depednency on
spdlog
, mostly for debugging/trace messages. Can we get rid of it? -
The documentation should mention early that this library introduces dependencies additional to
immer
: C++17 (or even C++20?),boost::hana
(other parts of boost as well?) and for the most partcereal
also. -
There are other changes I'd like to make to the docs, but I think I will do that once we're merged as it is easier than me making changes into your repo... Please remind me to give you write acces into the
immer
repo, so future PR's you can make directly from a branch inimmer
🙂 -
Talking about the documentation: it doesn't seem to indicate why the types need to be part of
boost::hana
by default. It could be an interesting thing to mention. Also I personally think the example code would be clearer ifBOOST_HANA_DEFINE_STRUCT
wasn't used, but instead we used the alternative syntax where the struct is defined normally and then separately the struct is lifted into Hana (it requires a few more characters, but the code is clearer in my opinion, particularly for people not familiar with Hana, as one doesn't need to understand the internals of Hana in order to know that the macro expands to mostly just a normal struct).
Overall excellent work! I think this is a massive use-case, it opens so many possibilities. And a unique feature not provided by any other immutable data-structures library I know of. Something worth writing a paper about probably! Looking forward to see this get more use and to evolve, so we can eventually treat it as a first-class citizen of immer
.
f8fc61f
to
c77ca34
Compare
Alright! Thank you for all this work! |
Structural sharing is traditionally preserved in memory only. When serializing multiple values to disk, these are linearized—for example, by writing out sequences as JSON arrays—losing whatever sharing may exist between these values. A similar thing happens when transforming data structures (as with
std::transform
or a functionalmap()
)Here we introduce an experimental module
immer::persist
that enables the serialization of pools of containers while preserving structural sharing. With it, we can also do in-memory transformation over large sets of data that considers and preserves structural sharing. This is novel technique, not present in any other implementation of persistent data structures. It has a few use-cases inside BRONZE.