Skip to content
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

flex_vector nothrow move constructible/assignable #246

Conversation

maierlars
Copy link
Contributor

@maierlars maierlars commented Dec 30, 2022

This is a result of #244.

This is just an idea how one could make the move constructor and assignment operator noexcept.
I used aligned_storage to statically reserve some memory for the nodes.

However, while I type this description I noticed a problem: I'm not familiar with the mechanism the frees memory. There is nothing that prevents the code from attempting to free the statically allocated buffers.
A possible solution would be to keep the ref count always above 0.

@codecov-commenter
Copy link

codecov-commenter commented Dec 30, 2022

Codecov Report

Merging #246 (b7756f5) into master (a5991ef) will increase coverage by 0.00%.
The diff coverage is 96.00%.

@@           Coverage Diff           @@
##           master     #246   +/-   ##
=======================================
  Coverage   90.50%   90.50%           
=======================================
  Files         119      119           
  Lines       12109    12121   +12     
=======================================
+ Hits        10959    10970   +11     
- Misses       1150     1151    +1     
Impacted Files Coverage Δ
immer/flex_vector.hpp 100.00% <ø> (ø)
immer/detail/rbts/node.hpp 79.32% <90.90%> (+0.06%) ⬆️
immer/detail/rbts/rrbtree.hpp 88.65% <100.00%> (+0.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Owner

@arximboldi arximboldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small comment inline, but in general, I really love this approach, I think this is the cleanest solution.

Re. avoiding "deallocation", ensuring the refcount is always above 0 is the right approach. It seems you are already doing that, so that looks good.

Also, further improvements that you could optionally include in this PR, or if you want, tackle in later further PRs.

  • It looks like copy-construction and assignment can also be made noexcept.
  • I'd love to see the same pattern applied to all other data-structures, I believe array, box, vector, map, set and table all could benefit from this technique.

Comment on lines 62 to 63
static std::aligned_storage_t<size, alignof(std::max_align_t)> storage;
static const auto empty_ = node_t::make_leaf_n_into(&storage, size, 0u);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer this:

static const auto empty_ = [] {
  static std::aligned_storage_t<size, alignof(std::max_align_t)> storage;
  return node_t::make_leaf_n_into(&storage, size, 0u);
}();

This hides storage inside the initialization of empty_ which I think could have some advantages (from an understandability POV, but it also makes it clear to the compiler that it only needs to access it once, during the protected initialization of empty_ itself.)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same self-executing lambda pattern can be used for the root above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good!

immer/detail/rbts/node.hpp Outdated Show resolved Hide resolved
@maierlars maierlars marked this pull request as ready for review January 5, 2023 10:53
@maierlars
Copy link
Contributor Author

How can we proceed with this PR? :)

@arximboldi
Copy link
Owner

I am happy to merge it. Just wasn't sure what your opinion is about the other improvements I suggested. I guess they could be tackled in a separate PR, would you like to do them? Should I just merge this branch as-is for now?

@maierlars
Copy link
Contributor Author

If you are happy with this PR, please merge it. For my work it is not urgent to have the other data-structures noexcept move-constr, however, I totally agree, that this should be done at some point. Maybe I find time to look into this over the weekend, but if someone else feels like doing it, please go ahead.

@arximboldi arximboldi merged commit 3c527e4 into arximboldi:master Jan 10, 2023
@arximboldi
Copy link
Owner

Sounds good to me Lars, merged!

Also, quite interesting the use-case for Arango DB. Curious about how you are using the library and what benefits it is providing. Happy to follow up per e-mail some time if you'd like to keep that a bit more private: [email protected]

@maierlars
Copy link
Contributor Author

Sure, there a multiple use cases for immer at ArangoDB, however, the flex vector is used primarily for an in-memory representation of a sequence of operations (which is also dumped to disk). For each shard we keep the history of the recent operations on documents in that shard and replicated the operations from the leader to its followers. While new log entries are appended, old entries are eventually compacted (removed) and we take snapshots of ranges for iteration or network messages regularly.

At the time we initially designed the prototype it just felt natural to use an immutable data structure for that. And until today we see no performance loss. After all, we are writing a database and want to replicate data, so the whole business is clearly dominated by disk and network io times. In fact, I personally have less mental load, I can just create a copy: it's cheap, it's not modified, its' noexcept, and no thread is blocked by that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants