Fix ring_buffer comparation logic

[ChaiByte]

Make sure to compare two ring buffers in ring-buffer-order.

Resolve: #511
CC: @grojo-ea @jhopkins-ea

[grojo-ea]

I don't think this is correct. Shouldn't this unconditionally return false if the ring buffers have different sizes?

[ChaiByte]

You are right，it is a stupid blunder. 😅 Fixed.

I have found that it may be possible to avoid calling size() here and instead use the eastl::identical interface for more precise semantics and readability. However, it seems that this interface is not mandated by the standard. Which approach do you think is better?

// Method A (Concise, but not mandated by the standard)
return eastl::identical(a.begin(), a.end(), b.begin(), b.end());

// Method B (Better complexity)
return (a.size() == b.size()) && eastl::equal(a.begin(), a.end(), b.begin());

// Method C (More clear than B, but need additional conditional check.)
if (a.size() != b.size())
    return false;
return eastl::equal(a.begin(), a.end(), b.begin());

I prefer method B.

And it seems redundant to separately getting size variables in current implement:

const auto sizeA = a.size();
const auto sizeB = b.size();

They are used just once.

[jhopkins-ea]

eastl::identical is equivalent to std::equal(first1, last1, first2, last2), ie. overloads 5-8 listed at cppref here. Method A and B have the same complexity from a Big O notation perspective. Please use method A.

[ChaiByte]

@jhopkins-ea Thank you for sharing this information with me.

I agree that method B and C have the same Big O complexity. The difference between them lies solely in the number of times the instructions are executed. and I trust the conditional branch should eventually be optimized away by the compiler.

Let's talking more about the method A and B:

EASTL/include/EASTL/algorithm.h

Lines 1866 to 1875 in 61c6a01

	template <typename InputIterator1, typename InputIterator2>
	EA_CPP14_CONSTEXPR inline bool equal(InputIterator1 first1, InputIterator1 last1, InputIterator2 first2)
	{
	for(; first1 != last1; ++first1, ++first2)
	{
	if(!(*first1 == *first2)) // Note that we always express value comparisons in terms of < or ==.
	return false;
	}
	return true;
	}

EASTL/include/EASTL/algorithm.h

Lines 1959 to 1969 in 61c6a01

	template <typename InputIterator1, typename InputIterator2>
	bool identical(InputIterator1 first1, InputIterator1 last1,
	InputIterator2 first2, InputIterator2 last2)
	{
	while((first1 != last1) && (first2 != last2) && (*first1 == *first2))
	{
	++first1;
	++first2;
	}
	return (first1 == last1) && (first2 == last2);
	}

Assume that here are two ring buffer rb1 (with size 1'000'000) and rb2 (with size 1'000'010) and we need to compare them:

RBVectorInt rb1 {0, ..., 0} // all elements are 0
RBVectorInt rb2 {0, ..., 1, 0, 0, 0, ..., 0,} // the 1'000'000th element is 1, others are 0

In this case, it need at least 1'000'000 times comparation to get result. But if we can get size information in advance, we can avoid redundant comparation.

[ChaiByte]

Let's see what cppref says in equal:

Complexity
5,7) At most min(last1 - first1, last2 - first2) applications of the predicate.
However, if InputIt1 and InputIt2 meet the requirements of LegacyRandomAccessIterator and last1 - first1 != last2 - first2 then no applications of the predicate are made (size mismatch is detected without looking at any elements).
2,4,6,8) same, but the complexity is specified as O(x), rather than "at most x".

From libstdc++-v3 we can find the optimization:

#if __cplusplus >= 201103L
  // 4-iterator version of std::equal<It1, It2> for use in C++11.
  template<typename _II1, typename _II2>
    _GLIBCXX20_CONSTEXPR
    inline bool
    __equal4(_II1 __first1, _II1 __last1, _II2 __first2, _II2 __last2)
    {
      using _RATag = random_access_iterator_tag;
      using _Cat1 = typename iterator_traits<_II1>::iterator_category;
      using _Cat2 = typename iterator_traits<_II2>::iterator_category;
      using _RAIters = __and_<is_same<_Cat1, _RATag>, is_same<_Cat2, _RATag>>;
      if (_RAIters())
	{
	  auto __d1 = std::distance(__first1, __last1);
	  auto __d2 = std::distance(__first2, __last2);
	  if (__d1 != __d2)
	    return false;
	  return _GLIBCXX_STD_A::equal(__first1, __last1, __first2);
	}

      for (; __first1 != __last1 && __first2 != __last2;
	  ++__first1, (void)++__first2)
	if (!(*__first1 == *__first2))
	  return false;
      return __first1 == __last1 && __first2 == __last2;
    }

It becames a X-Y problem: current eastl::identical is not effient enough when input iterators meet LegacyRandomAccessIterator requirements. It's another issue. If it was fixed, just call eastl::identical(a.begin(), a.end(), b.begin(), b.end()) is enough (Method A).

So we need check a.size() == b.size() to determine whether calling them or not (Method B).

[grojo-ea]

eastl::identical is equivalent to std::equal(first1, last1, first2, last2), ie. overloads 5-8 listed at cppref here.

I just realized EASTL doesn't have the equal overloads taking 2 iterator pairs. Also, given that eastl::identical doesn't do the range size optimization for random access iterators it's technically not the same.

I think I'd prefer us to add those overloads (with the correct optimizations for random access iterators) than to proliferate the usage of eastl::identical (which arguably should be deprecated in favor of the new equal overloads). But this is out of the scope of this PR.

Given the current state of things, option B is probably the best we can do.

[ChaiByte]

I just realized EASTL doesn't have the equal overloads taking 2 iterator pairs. Also, given that eastl::identical doesn't do the range size optimization for random access iterators it's technically not the same.

I can provide more information:

When I first found the eastl::identical interface, I didn't directly realize that it is an overload of the equal function that accepts different parameters. I subconsciously think that this is a "more efficient" equal, and the user must ensure that the length is equal before calling. for better performance.

Thanks to @jhopkins-ea for reminding me to establish right perception. XD

I think I'd prefer us to add those overloads (with the correct optimizations for random access iterators) than to proliferate the usage of eastl::identical (which arguably should be deprecated in favor of the new equal overloads). But this is out of the scope of this PR.

It would be great if this could be done, I tried to write eastl::equal(a.begin(), a.end(), b.begin(), b.end()) at first but found that it could not be compiled , that feels strange, because it can be done in other STL implementations, and it will be a better experience to be consistent.

Given the current state of things, option B is probably the best we can do.

Glad to hear that, are there any other suggestions? I think it's time to merge.

[jhopkins-ea]

Good pick up on the complexity differences between identical and equal. We should in the future deprecate identical in favour of new equal overloads that have the complexity guarantees.

This is a minor issue, but container.size()/std::size() does not have a guaranteed constant time, as opposed to ranges::size() (see here). If we used a eastl::list as the backing container this would be a linear time operation, because eastl::list does not store the size (unlike a std::list). Really, that's just more of a reason to add the C++14 equal overloads.

Thanks for your contribution.

[grojo-ea]

Thanks for the changes, the contribution, and the discussion.