Page MenuHomePhabricator

Make `vector` unconditionally move elements when exceptions are disabled.
ClosedPublic

Authored by EricWF on May 21 2019, 7:08 PM.

Details

Summary

std::vector<T> is free choose between using copy or move operations when it needs to resize. The standard only candidates that the correct exception safety guarantees are provided. When exceptions are disabled these guarantees are trivially satisfied. Meaning vector is free to optimize it's implementation by moving instead of copying.

This patch makes std::vector unconditionally move elements when exceptions are disabled.

This optimization is conforming according to the current standard wording.

There are concerns that moving in -fno-noexceptionsmode will be a surprise to users. For example, a user may be surprised to find their code is slower with exceptions enabled than it is disabled. I'm sympathetic to this surprised, but I don't think it should block this optimization.

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF created this revision.May 21 2019, 7:08 PM

I'm not particularly in favor of this patch.
There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

I not that fond of -fno-exceptions in general, and I think that special-casing vector here is the wrong approach.
If we really want to embrace this (and I don't), I think we should change move_if_noexcept (and the other noexcept type traits) to return true when exceptions are disabled.

Also, these functions __construct_backward and __construct_forwards should be free functions, not non-standard extensions to allocator_traits (but that's existing code)

include/utility
284 ↗(On Diff #200625)

Why an enum?
How about:

#ifdef _LIBCPP_NO_EXCEPTIONS
_LIBCPP_BOOL_CONSTANT(true) __exceptions_disabled;
#else
_LIBCPP_BOOL_CONSTANT(false) __exceptions_disabled;
#endif
EricWF marked an inline comment as done.May 22 2019, 11:15 AM

I'm not particularly in favor of this patch.
There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

Louis and I talked more offline and there was some consensus that this minimal change was more acceptable.
But if there are additional concerns they can be raised here.

I not that fond of -fno-exceptions in general, and I think that special-casing vector here is the wrong approach.
If we really want to embrace this (and I don't), I think we should change move_if_noexcept (and the other noexcept type traits) to return true when exceptions are disabled.

You're proposing a non-standard and non-conforming extension. This change is not an extension. It is a conforming implementation of vector that is better optimized.
This change is orthogonal to the suggested non-conforming extension, and discussion about the extension is better had elsewhere.

Also, these functions __construct_backward and __construct_forwards should be free functions, not non-standard extensions to allocator_traits (but that's existing code)

They have an interesting relationship with the allocator, but I agree they're mis-placed.

include/utility
284 ↗(On Diff #200625)

It's just a quick way to stash a value without having to worry about introducing a name with linkage.

mclow.lists added a comment.EditedMay 23 2019, 9:03 AM

I'm not particularly in favor of this patch.
There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

Louis and I talked more offline and there was some consensus that this minimal change was more acceptable.
But if there are additional concerns they can be raised here.

I not that fond of -fno-exceptions in general, and I think that special-casing vector here is the wrong approach.
If we really want to embrace this (and I don't), I think we should change move_if_noexcept (and the other noexcept type traits) to return true when exceptions are disabled.

You're proposing a non-standard and non-conforming extension. This change is not an extension. It is a conforming implementation of vector that is better optimized.
This change is orthogonal to the suggested non-conforming extension, and discussion about the extension is better had elsewhere.

Are you seriously suggesting that any behavior under -fno-exceptions can be considered conforming?

There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

Louis and I talked more offline and there was some consensus that this minimal change was more acceptable.
But if there are additional concerns they can be raised here.

I don't think this should be committed until there is consensus on the direction, and when there's contention the usual forum for establishing consensus is the dev list. Can you restart the discussion on libcxx-dev, or start a new one there?

The approach I was more comfortable with was defining a macro that would be somewhat like noexcept(expression), but that would always return true when exceptions are disabled. This would be a library emulation of the compiler change we talked about on the list.

test/libcxx/utilities/utility/forward/extended_move_if_noexcept.sh.cpp
27–28 ↗(On Diff #200625)

This comment should be in the header where __extended_move_if_noexcept is defined, not in this test.

zoecarver added inline comments.
include/utility
298 ↗(On Diff #200625)

Missing public:. In C++03 these won't be public member types.

EricWF updated this revision to Diff 205965.Jun 21 2019, 4:19 AM

Sorry, I think the previous patch was misleading. My ambitions don't extend beyond this small performance fix. There are no other places in libc++ that conditionally move.

I've removed __extended_move_if_noexcept because we've agreed the general abstraction it implements isn't what we want and isn't good generally.

Is this localized performance fix more acceptable? I don't think it violates the spirit of the previous conversation.

ldionne accepted this revision.Aug 5 2019, 10:32 AM
This revision is now accepted and ready to land.Aug 5 2019, 10:32 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 12:02 PM

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Is this breakage intentional?

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Is this breakage intentional?

I don't think this was intentional, and I think your example should be valid. I added the test you provided to the test suite in https://reviews.llvm.org/rCXX371067, and I temporarily reverted this commit in https://reviews.llvm.org/rCXX371068. We can take a look once @EricWF is back from vacation.

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Is this breakage intentional?

I don't think this was intentional, and I think your example should be valid. I added the test you provided to the test suite in https://reviews.llvm.org/rCXX371067, and I temporarily reverted this commit in https://reviews.llvm.org/rCXX371068. We can take a look once @EricWF is back from vacation.

Thanks for the revert!