-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Optimize any::swap
#5710
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
Optimize any::swap
#5710
Conversation
c3497f0
to
5567c9c
Compare
Thanks! 😻 I pushed a change to further simplify and optimize Here are benchmarks comparing
I believe that v3 passes our test coverage and handles self-swap scenarios equally well. |
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for optimizing and benchmarking this! ⏱️ 🏎️ 🎉 |
Initially, I didn't dare to remove that additional copy in Summary:
(I'm only talking about cursed swaps here. All these versions are supposed to handle normal swaps correctly (including normal self-swaps).) Suppose
v2 impl (equivalent version): any _Old_this = _STD move(*this);
{
any _Old_that = _STD move(_That);
reset();
_Move_from(_Old_that);
}
_That.reset();
_That._Move_from(_Old_this); Effects: // If `a.swap(b)` happens first:
any old_a = move(a); // old_a contains (owns) &b, a is moved-from (exchanged / emptied).
{
any old_b = move(b); // contains move(c), b is moved-from.
a.reset(); // noop.
a._Move_from(old_b); // a contains move(move(c))
}
b.reset(); // b is emptied.
b._Move_from(old_a); // b owns &b. (!!)
// If `b.swap(a)` happens first:
any old_b = move(b); // contains move(c), b is moved-from.
{
any old_a = move(a); // owns &b, a is emptied.
b.reset(); // b is emptied.
b._Move_from(old_a); // b owns &b. (!!)
}
a.reset(); // noop.
a._Move_from(old_b); // a contains move(move(c)). v3 impl: any _Old = _STD move(*this);
reset();
_Move_from(_That);
_That.reset();
_That._Move_from(_Old); Effects: // If `a.swap(b)` happens first:
any old_a = move(a); // old_a owns &b, a is emptied.
a.reset(); // noop.
a._Move_from(b); // a contains move(c), b is moved-from.
b.reset(); // b is emptied.
b._Move_from(old_a); // b owns &b. (!!)
// If `b.swap(a)` happens first:
any old_b = move(b); // old_b contains move(c), b is moved-from.
b.reset(); // b is emptied.
b._Move_from(a); // b owns &b, a is emptied. (!!)
a.reset(); // noop.
a._Move_from(old_b); // b contains move(c). In all these cases (either v2 or v3, either This seems a serious problem, but it turns out that, at least this pr doesn't make anything worse - the original Take the following examples for example. On my visual studio (using the original // 1
#include<any>
int main()
{
for (;;) {
std::any a = std::make_any<std::any>(42);
std::any& b = *std::any_cast<std::any>(&a);
a.swap(b);
}
}
// 2
#include<any>
int main()
{
std::any a = std::make_any<std::any>(42);
std::any& b = *std::any_cast<std::any>(&a);
a.swap(b);
b.~any();
} Initially, I thought this may be due to the implementation chooses to hold data in-place for trivial/small types. However, it turns out even if std::swap(a.p, b.p);
// (and swap type info) Note that after this, the same issue happens - now I believe these cursed swaps cannot be well-defined. Conceptually,
If the swap must be defined for As to the current C++ standard, it only says the effect of
(Copy and move assignments are not cursed likewise as they are explicitly defined in terms of creating a temporary then swap.) |
Thanks for the careful analysis! |
This pr removes an unnecessary copy in
any::swap
.Hopefully fixes #5698.
Benchmark result: