[libcxx-commits] [PATCH] D133661: [libc++] Improve binary size when using __transaction

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

In D133661#3791177 <https://reviews.llvm.org/D133661#3791177>, @hans wrote:

> Since D128146 <https://reviews.llvm.org/D128146> we've switched to compressed debug info sections for that binary, so the numbers are slightly different, but with the current patch I measure an increase of 2.45 MB (0.16%).

Thanks for the numbers! I think this might be the best we can do from the library side, apart from adding some additional `_LIBCPP_NODEBUG` attributes (but that also kind of breaks the debugging experience). This must be caused by additional instantiations of the class template.



================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:536
   auto __destruct_first = __first2;
-  try {
-#endif
+  auto __guard =
+      std::__make_transaction(_AllocatorDestroyRangeReverse<_Alloc, _Iter2>(__alloc, __destruct_first, __first2));
----------------
Mordante wrote:
> Can you please stop using auto everywhere? This has been requested several time by several reviewers. It really makes it unneeded hard to review code. The LLVM developer policy explains why this style isn't wanted
> https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable.
> 
> (In library code there might be reasons to use `auto` a bit more often, but this isn't one of these places.)
> 
> 
> 
I fully agree that we shouldn't blindly use `auto`, I think it generally obscures the code. IMO these "almost always use auto" advice should never have been given, they were just too easy to mis-interpret and exaggerate on.

That being said, the alternative here is:

```
std::__transaction<_AllocatorDestroyRangeReverse<_Alloc, _Iter2> > __guard(_AllocatorDestroyRangeReverse<_Alloc, _Iter2>(__alloc, __destruct_first, __first2));
```

IMO, since we have a `std::__make_transaction` function, we don't gain much from repeating the type. This is just my .02, I don't really care about this specific instance. I think this one is reasonable, but I agree with @Mordante in the general case about not abusing `auto` (I personally don't remember seeing @philnik abuse it, though).


================
Comment at: libcxx/include/__utility/transaction.h:90-104
+#else
+template <class _Rollback>
+struct __transaction {
+  __transaction() = delete;
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __transaction(_Rollback) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __transaction(__transaction&&)
+      _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value) {}
----------------
Mordante wrote:
> huixie90 wrote:
> > having this `__transaction` class as a noop class in `_LIBCPP_NO_EXCEPTIONS` mode might cause confusions, because the class name does not indicate anything to do with exceptions (and could be misused as a general purpose `scope_guard`).
> > 
> > what about having a function `__make_exception_guard`, which returns the `__transaction` class in the exception mode, and returns a `__noop_transaction` class in the noexception mode?
> +1 for the confusion. What makes it more confusing is the usage of `_NOEXCEPT` in this class. Isn't that useless when exceptions are disabled? Please also some comment why this class it to be used as a no-op.
> 
> I'm not even sure whether we really want this class. I expect a transaction that is not committed when it goes out of scope to do a rollback.
> ```
> void __foo() {
>   __transaction __t;
>   ... // do stuff
>   if(!__happy())
>     return;
>   __t.__complete();
> }
> 
> ```
> In that way this class behaves different depending on whether or not exceptions are enabled.
> 
> Based on the failure in the 'No exception' CI the behaviour above was intended to be working.
I fully agree with the other reviewers, I think it would be wrong to make `__transaction` a no-op based on whether exceptions are enabled. However, I think it's only an issue because of the semantics associated to the name `__transaction`, which does not imply something related to exceptions.

I'd suggest:

```
template <class _Rollback>
struct __noop_transaction {
  __noop_transaction() = delete;
  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __noop_transaction(_Rollback) {}
  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __noop_transaction(__transaction&&)
      _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value) {}
  __noop_transaction(const __transaction&) = delete;
  __noop_transaction& operator=(const __transaction&) = delete;
  __noop_transaction& operator=(__transaction&&) = delete;

  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG void __complete() _NOEXCEPT {}
  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG ~ __noop_transaction() {}
};


#ifndef _LIBCPP_NO_EXCEPTIONS
template <class _Rollback>
using __exception_guard = __transaction<_Rollback>;
#else
template <class _Rollback>
using __exception_guard = __noop_transaction<_Rollback>;
#endif

template <class _Rollback>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __exception_guard<_Rollback> __make_exception_guard(_Rollback __rollback) {
  return __exception_guard <_Rollback>(std::move(__rollback));
}
```

Also, you should use `__exception_guard` instead of `__transaction` in most existing places where `__transaction` is used right now.


================
Comment at: libcxx/include/__utility/transaction.h:105
+#endif
 
 template <class _Rollback>
----------------
Let's also add some really simple tests for this new `__exception_guard` class.


================
Comment at: libcxx/include/__utility/transaction.h:111
 
 _LIBCPP_END_NAMESPACE_STD
 
----------------
Not attached: can you please link to https://github.com/llvm/llvm-project/issues/56783 somewhere?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133661/new/

https://reviews.llvm.org/D133661