gamescope icon indicating copy to clipboard operation
gamescope copied to clipboard

CloseAllFds: use ranges::any_of() instead of find() for better codegen

Open sharkautarch opened this issue 7 months ago • 0 comments

@Joshua-Ashton I heard you were annoyed by how std::find() was getting compiled for CloseAllFds()

I looked into this issue, and found that using std::ranges::find() or std::ranges::any_of() results in way simpler codegen compared to their non-range counterparts: https://godbolt.org/z/dEnMcq1Kf (note: for all versions of CloseAllFds() given in the godbolt link, I put in

if (pBeginExclude == pEndExclude) {
            __builtin_unreachable();
}

the sole purpose of this is to eliminate one conditional branch, to simplify control flow, in order to make it easier to compare all of the versions)

Now for a tidbit that's more for satisfying your curiosity as to what could possibly be causing std::find() to lead to such convoluted assembly/machine code: It turns out that it wasn't the compiler that was causing this, but actually the implementation of std::find(). On my archlinux system, the implementation of std::find() is in /usr/include/14.1.1/bits/stl_algo.h but it actually just calls std::__find_if, which is in /usr/include/14.1.1/bits/stl_algobase.h the version of __find_if that's called in the godbolt examples, and in the 'real' version of CloseAllFds() is:

 /// This is an overload used by find algos for the RAI case.
  template<typename _RandomAccessIterator, typename _Predicate>
    _GLIBCXX20_CONSTEXPR
    _RandomAccessIterator
    __find_if(_RandomAccessIterator __first, _RandomAccessIterator __last,
              _Predicate __pred, random_access_iterator_tag)
    {
      typename iterator_traits<_RandomAccessIterator>::difference_type
        __trip_count = (__last - __first) >> 2;

      for (; __trip_count > 0; --__trip_count)
        {
          if (__pred(__first))
            return __first;
          ++__first;

          if (__pred(__first))
            return __first;
          ++__first;

          if (__pred(__first))
            return __first;
          ++__first;

          if (__pred(__first))
            return __first;
          ++__first;
        }

      switch (__last - __first)
        {
        case 3:
          if (__pred(__first))
            return __first;
          ++__first;
          // FALLTHRU
        case 2:
          if (__pred(__first))
            return __first;
          ++__first;
          // FALLTHRU
        case 1:
          if (__pred(__first))
            return __first;
          ++__first;
          // FALLTHRU
        case 0:
        default:
          return __last;
        }
    }

I'm not sure why exactly, but I guess ranges::find() and ranges::any_of() don't use that convoluted implementation...

sharkautarch avatar Jul 14 '24 15:07 sharkautarch