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

Assertion failure with immer::flex_vector_transient #16

Closed
ddcc opened this issue Sep 15, 2017 · 2 comments
Closed

Assertion failure with immer::flex_vector_transient #16

ddcc opened this issue Sep 15, 2017 · 2 comments
Labels

Comments

@ddcc
Copy link

ddcc commented Sep 15, 2017

I have a small dummy program that appends and erases unsigned integers from an immer::flex_vector_transient. By default, I understand that the container will use immer::refcount_policy (with immer::no_transience_policy), and everything works fine.

Since the keys are just unsigned integers (32-bits), I'd like to disable reference counting in order to measure the runtime overhead. By changing the container to use immer::no_refcount_policy, this automatically selects immer:: gc_transience_policy, which leaks memory without the Boehm GC (as stated in the documentation).

I don't care about the memory leak since this is just a test program, but I encounter an assertion failure at runtime that is unexpected:
test: ../../immer/immer/detail/rbts/position.hpp:160: leaf_sub_pos<NodeT> immer::detail::rbts::make_leaf_sub_pos(NodeT *, immer::detail::rbts::count_t) [NodeT = immer::detail::rbts::node<unsigned int, immer::memory_policy<immer::free_list_heap_policy<immer::cpp_heap, 1024>, immer::no_refcount_policy, immer::gc_transience_policy, false, false>, 5, 6>]: Assertion 'count <= branches<NodeT::bits_leaf>' failed.

Is this supposed to occur? If I manually choose immer::no_transience_policy, the assertion doesn't fail. This is the test program: test.txt

arximboldi added a commit that referenced this issue Sep 16, 2017
When we copy a transient we are just doing a shallow copy, thus
creating "aliases" of the nodes.  This means that the copied-from
object should not be able to mutate its recently created nodes
anymore, since they are now visible elsewhere.  Thus, we need to reset
its own token.

We also reset the token of the copied-into object.  This is because
otherwise we could also reach a similar situation by moving an object,
then copying into it.  This could be otherwise solved by "swapping"
the tokens during a move-assignment or move-construction, but in that
case we would need to make the move-constructor noexcept (to allocate
a new token for it).  With the current approach the moves are cheaper
and, more importantly, noexcept.

This commit fixes issue #16
arximboldi added a commit that referenced this issue Sep 16, 2017
When we copy a transient we are just doing a shallow copy, thus
creating "aliases" of the nodes.  This means that the copied-from
object should not be able to mutate its recently created nodes
anymore, since they are now visible elsewhere.  Thus, we need to reset
its own token.

We also reset the token of the copied-into object.  This is because
otherwise we could also reach a similar situation by moving an object,
then copying into it.  This could be otherwise solved by "swapping"
the tokens during a move-assignment or move-construction, but in that
case we would need to make the move-constructor noexcept (to allocate
a new token for it).  With the current approach the moves are cheaper
and, more importantly, noexcept.

This commit fixes issue #16
@arximboldi
Copy link
Owner

Thanks a lot @ddcc, good catch!
The problem was that copying the transient was not correctly handling the tokens that are used internally to track ownership in the gc_transience_policy, thus the transient was updating in-place nodes that were visible via two different variables... (v2 and v inside erase).
I already made a fix, will merge as soon as Travis gives a green light.

@arximboldi arximboldi added the bug label Sep 16, 2017
@ddcc
Copy link
Author

ddcc commented Sep 16, 2017

Thanks for the quick fix!

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

No branches or pull requests

2 participants