Limit Debug mode impact: overload __niter_base

[Jonathan Wakely]

On 26/06/18 17:03 +0100, Jonathan Wakely wrote:
>On 18/06/18 23:01 +0200, François Dumont wrote:
>>Hi
>>
>>Â Â Â  I abandon the idea of providing Debug algos, it would be too 
>>much code to add and maintain. However I haven't quit on reducing 
>>Debug mode performance impact.
>>
>>Â Â Â  So this patch make use of the existing std::__niter_base to get 
>>rid of the debug layer around __gnu_debug::vector<>::iterator so 
>>that __builtin_memmove replacement can take place.
>>
>>Â Â Â  As _Safe_iterator<> do not have a constructor taking a pointer I 
>>change algos implementation so that we do not try to instantiate the 
>>iterator type ourselves but rather rely on its operators + or -.
>>
>>Â Â Â  The small drawback is that for std::equal algo where we can't 
>>use the __glibcxx_can_increment we need to keep the debug layer to 
>>make sure we don't reach past-the-end iterator. So I had to remove 
>>usage of __niter_base when in Debug mode, doing so it also disable 
>>potential usage of __builtin_memcmp when calling std::equal on 
>>std::__cxx1998::vector<> iterators. A rather rare situation I think.

We don't need to give up all checks in std::equal, we can do this:

@@ -1044,7 +1085,13 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
            typename iterator_traits<_II1>::value_type,
            typename iterator_traits<_II2>::value_type>)
       __glibcxx_requires_valid_range(__first1, __last1);
-
+#ifdef _GLIBCXX_DEBUG
+      typedef typename iterator_traits<_II1>::iterator_category _Cat1;
+      typedef typename iterator_traits<_II2>::iterator_category _Cat2;
+      if (!__are_same<_Cat1, input_iterator_tag>::__value
+         && __are_same<_Cat2, random_access_iterator_tag>::__value)
+       __glibcxx_requires_can_increment_range(__first1, __last1, __first2);
+#endif
       return std::__equal_aux(std::__niter_base(__first1),
                              std::__niter_base(__last1),
                              std::__niter_base(__first2));



>>Â Â Â  Note that I don't know how to test that __builtin_memmove has 
>>been indeed used. So I've been through some debug sessions to check 
>>that.
>
>The attached patch (not fully tested) seems to be a much simpler way
>to achieve the same thing. Instead of modifying all the helper
>structs, just define a new function to re-wrap the result into the
>desired iterator type.
>
>
>>diff --git a/libstdc++-v3/include/debug/stl_iterator.h b/libstdc++-v3/include/debug/stl_iterator.h
>>index a6a2a76..eca7203 100644
>>--- a/libstdc++-v3/include/debug/stl_iterator.h
>>+++ b/libstdc++-v3/include/debug/stl_iterator.h
>>@@ -120,4 +120,19 @@ namespace __gnu_debug
>>#endif
>>}
>>
>>+#if __cplusplus >= 201103L
>>+namespace std
>>+{
>>+_GLIBCXX_BEGIN_NAMESPACE_VERSION
>>+
>>+template<typename _Iterator, typename _Container, typename _Sequence>
>>+    _Iterator
>>+    __niter_base(const __gnu_debug::_Safe_iterator<
>>+		 __gnu_cxx::__normal_iterator<_Iterator, _Container>,
>>+		 _Sequence>&);
>>+
>>+_GLIBCXX_END_NAMESPACE_VERSION
>>+}
>>+#endif
>
>Why is this overload only defined for C++11 and later? I defined it
>unconditionally in the attached patch.
>
>What do you think?

Here's a complete patch that passes all tests in normal mode and
causes no regressions in debug mode (we already have some debug test
failing).

I wondered whether we need another overload of __wrap_iter for
handling move_iterator, but I think the first overload works OK.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 6933 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20180627/6ad73f3f/attachment.bin>