-
Notifications
You must be signed in to change notification settings - Fork 100
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
recursive variant move construction/assignment complexity #139
base: master
Are you sure you want to change the base?
Conversation
I decided to turn this PR into an actual fix for the issue demonstrated by the added benchmark. Rather than going the "make This would be a set-back for other variant implementations that optimize same-type assignment, but not for this one. |
This is the simplest way to achieve constant-time[1] complexity of variant move construction and assignment. [1] Constant with respect to the size of the recursive structure held by the variant object; it's still linear in the number of alternative types in the variant type.
cc5e553
to
57d9c7d
Compare
Hmm, just a thought: you might want to rename the "variant_helper::move" function to Or |
Another thought: Your patch is a lot smaller and simpler than I expected :D For instance, suppose I have some code
In current code, the last three lines of code will all end up invoking move assignment operator of std::string. After this patch, it's sometimes the move constructor instead, because after c and b are moved from their things get destroyed. The reason C++ has copy assignment operators and move assignment operators is that it may in some cases be more efficient than simply destroying and using a constructor. The object may need to acquire resources, or populate a small lookup table or something. In most cases though it's not a big difference. Also, your way of doing it is certainly valid under the usual definition of move semantics, the programmer can only assume a "valid state" after the move. I wonder if there's any non-artificial examples that would be slowed down by this. Potentially, you could try to make it so that it only does the "move and destroy" logic when the type is a |
... destroy the target first, then move-construct a new string. variant/include/mapbox/variant.hpp Line 629 in d2588a8
After this PR, they receive an already-destroyed target, so the call to
Yes, that's what I was aiming for, but then noticed that assignment unconditionally destroys the target first, so until that is changed (and I didn't want to extend the scope of this PR), it wouldn't buy much. |
Hmm right you are, that is a quirk of mapbox variant I guess |
Look at the times for "construction" of large trees.
With
recursive_wrapper
a 5x increase in tree size results in1485368 / 57397 > 25x
increase in running time.With
unique_ptr
the increase in running time is only171 / 41 > 4x
.There's one strange surprise, though. The first timing below TYPE is from an existing calculation test, and
unique_ptr
is almost twice as fast after the change (which doesn't touch any of that code), whereas before the change, there was no difference:That's with
gcc-6.2
. I previously tried with 4.8 and 5.4 and there were differences but not really convincing;recursive_wrapper
was faster, then I slightly changed the code to only construct onetest::calculator
instead of constructing temporaries for every operation, and suddenlyunique_ptr
was faster; then I reverted that change, added thebench_large_tree
function (without calling it) and the times were almost identical.