Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 16/11/2017 12:41, Petr Ovtchenkov wrote:

This part of code is from SGI, so I suspect that nobody here really
measure performance difference between "bufferred" and "non-buffered"
implementations.
Here where? The GNU libstdc++-v3 implementation? Certainly we did, as I 
tried to tell you the issue - the tradeoff between a more "correct", 
simpler, and certainly easier to synchronize implementation and better 
performance in some cases - isn't new. Please carry out a through search 
of Bugzilla and mailing list, there must be something recorded. I'll see 
if I can help about that, it's been a while.


Paolo.

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 12:29:37 +0100
Paolo Carlini  wrote:

> Hi,
> 
> On 16/11/2017 12:03, Petr Ovtchenkov wrote:
> > On Thu, 16 Nov 2017 10:39:02 +0100
> > Paolo Carlini  wrote:
> >
> >> Hi,
> >>
> >> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> >>> Is we really worry about frozen sizeof of instantiated template?
> >> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> >> under "Prohibited Changes", point 8.
> >>
> >> Of course removing the buffering has performance implications too -
> >> that's why it's there in the first place!
> > "buffering" here is a secondary buffering (after streambuf).
> > No relation to performance, but place for incoherence with
> > state of attached streambuf.
> It depends, we may be dealing with an unbuffered stream. For sure at the 
> time we measured a performance impact in some cases, likewise whoever 
> implemented it in the first place (not me) otherwise, again, why bothering?

This part of code is from SGI, so I suspect that nobody here really 
measure performance difference between "bufferred" and "non-buffered"
implementations. Just because we have only implementation
with _M_c in isreambuf_iterator.

> 
> Paolo.
> 

--

   - ptr

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 16/11/2017 12:03, Petr Ovtchenkov wrote:

On Thu, 16 Nov 2017 10:39:02 +0100
Paolo Carlini  wrote:


Hi,

On 16/11/2017 06:31, Petr Ovtchenkov wrote:

Is we really worry about frozen sizeof of instantiated template?

Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
under "Prohibited Changes", point 8.

Of course removing the buffering has performance implications too -
that's why it's there in the first place!

"buffering" here is a secondary buffering (after streambuf).
No relation to performance, but place for incoherence with
state of attached streambuf.
It depends, we may be dealing with an unbuffered stream. For sure at the 
time we measured a performance impact in some cases, likewise whoever 
implemented it in the first place (not me) otherwise, again, why bothering?


Paolo.

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 10:39:02 +0100
Paolo Carlini  wrote:

> Hi,
> 
> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> > Is we really worry about frozen sizeof of instantiated template? 
> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html 
> under "Prohibited Changes", point 8.
> 
> Of course removing the buffering has performance implications too - 
> that's why it's there in the first place!

"buffering" here is a secondary buffering (after streambuf).
No relation to performance, but place for incoherence with
state of attached streambuf.

Of cause, I can spend time to measure the difference.

The main point of this patch series is avoidance of lost link between
streambuf and istream_iterator when istream_iterator see eof.
Implementations that forget about attached streambuf after
istream_iterator see eof (or lost synchronization with attached
streambuf) violate principles of C++ objects life cycle.
From practical point of view, such implementation block usage of
istream_iterator for sockets, ttys, etc. --- only non-modified
files remains in scope of application.


> - which I remember we 
> investigated a bit again in the past when somebody reported that a few 
> implementations had it other did not. But I can't say to have followed 
> all the (recently uncovered) conformance implications, it could well be 
> that we cannot be 100% conforming to the letter of the current standard 
> while taking advantage of a buffering mechanism. Jonathan will provide 
> feedback.
> 
> Paolo.

--

  - ptr

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 16/11/2017 06:31, Petr Ovtchenkov wrote:
Is we really worry about frozen sizeof of instantiated template? 
Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html 
under "Prohibited Changes", point 8.


Of course removing the buffering has performance implications too - 
that's why it's there in the first place! - which I remember we 
investigated a bit again in the past when somebody reported that a few 
implementations had it other did not. But I can't say to have followed 
all the (recently uncovered) conformance implications, it could well be 
that we cannot be 100% conforming to the letter of the current standard 
while taking advantage of a buffering mechanism. Jonathan will provide 
feedback.


Paolo.

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

On Wed, 15 Nov 2017 22:31:11 +0100
Paolo Carlini  wrote:

> Hi,
> 
> On 15/11/2017 11:48, Petr Ovtchenkov wrote:
> > Ask associated streambuf for character when needed instead of
> > accumulate it in istreambuf_iterator object.
> >
> > Benefits from this:
> >- minus one class member in istreambuf_iterator
> >- trivial synchronization of states of istreambuf_iterator
> >  and associated streambuf
> > ---
> >   libstdc++-v3/include/bits/streambuf_iterator.h | 34 
> > --
> >   1 file changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
> > b/libstdc++-v3/include/bits/streambuf_iterator.h index 08fb13b..203da9d 
> > 100644
> > --- a/libstdc++-v3/include/bits/streambuf_iterator.h
> > +++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> > @@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > // NB: This implementation assumes the "end of stream" value
> > // is EOF, or -1.
> > mutable streambuf_type* _M_sbuf;
> > -  mutable int_type _M_c;
> Obviously this would be an ABI-breaking change, which certainly we don't 
> want. Unless I missed a detailed discussion of the non-trivial way to 
> avoid it in one of the recent threads about these topics...

Is we really worry about frozen sizeof of instantiated template?
(Removed private template member).

If yes, than

   int_type __dummy;

is our all.

> 
> Paolo.

--

   - ptr

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 15/11/2017 11:48, Petr Ovtchenkov wrote:

Ask associated streambuf for character when needed instead of
accumulate it in istreambuf_iterator object.

Benefits from this:
   - minus one class member in istreambuf_iterator
   - trivial synchronization of states of istreambuf_iterator
 and associated streambuf
---
  libstdc++-v3/include/bits/streambuf_iterator.h | 34 --
  1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h 
b/libstdc++-v3/include/bits/streambuf_iterator.h
index 08fb13b..203da9d 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
mutable streambuf_type* _M_sbuf;
-  mutable int_type _M_c;
Obviously this would be an ABI-breaking change, which certainly we don't 
want. Unless I missed a detailed discussion of the non-trivial way to 
avoid it in one of the recent threads about these topics...


Paolo.

[PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

Ask associated streambuf for character when needed instead of
accumulate it in istreambuf_iterator object.

Benefits from this:
  - minus one class member in istreambuf_iterator
  - trivial synchronization of states of istreambuf_iterator
and associated streambuf
---
 libstdc++-v3/include/bits/streambuf_iterator.h | 34 --
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h 
b/libstdc++-v3/include/bits/streambuf_iterator.h
index 08fb13b..203da9d 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // NB: This implementation assumes the "end of stream" value
   // is EOF, or -1.
   mutable streambuf_type*  _M_sbuf;
-  mutable int_type _M_c;
 
 public:
   class proxy
   {
   friend class istreambuf_iterator;
   private:
-  proxy(int_type c, streambuf_type*sbuf_) :
+  proxy(int_type c, streambuf_type* sbuf_) :
   _M_c(c),
   _M_sbuf(sbuf_)
   { }
   int_type _M_c;
-  streambuf_type*  _M_sbuf;
+  streambuf_type* _M_sbuf;
 
   public:
   char_type
@@ -118,7 +117,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 public:
   ///  Construct end of input stream iterator.
   _GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
-  : _M_sbuf(0), _M_c(traits_type::eof()) { }
+  : _M_sbuf(0) { }
 
 #if __cplusplus >= 201103L
   istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
@@ -128,15 +127,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   ///  Construct start of input stream iterator.
   istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-  : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+  : _M_sbuf(__s.rdbuf()) { }
 
   ///  Construct start of streambuf iterator.
   istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-  : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+  : _M_sbuf(__s) { }
 
   ///  Construct start of istreambuf iterator.
   istreambuf_iterator(const proxy& __p) _GLIBCXX_USE_NOEXCEPT
-  : _M_sbuf(__p._M_sbuf), _M_c(traits_type::eof()) { }
+  : _M_sbuf(__p._M_sbuf) { }
 
   ///  Return the current character pointed to by iterator.  This returns
   ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -147,11 +146,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
-   __glibcxx_requires_cond(!_M_at_eof(),
+   int_type __tmp = _M_get();
+   
__glibcxx_requires_cond(!traits_type::eq_int_type(__tmp,traits_type::eof()),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
-#endif
+   return traits_type::to_char_type(__tmp);
+#else
return traits_type::to_char_type(_M_get());
+#endif
   }
 
   /// Advance the iterator.  Calls streambuf.sbumpc().
@@ -172,7 +174,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
 #endif
-   _M_c = traits_type::eof();
  }
return *this;
   }
@@ -181,17 +182,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   proxy
   operator++(int)
   {
-_M_get();
-   __glibcxx_requires_cond(_M_sbuf
-   && 
!traits_type::eq_int_type(_M_c,traits_type::eof()),
+int_type c = _M_get();
+   __glibcxx_requires_cond(!traits_type::eq_int_type(c,traits_type::eof()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
 
-   proxy __old(_M_c, _M_sbuf);
+   proxy __old(c, _M_sbuf);
if (_M_sbuf)
  {
_M_sbuf->sbumpc();
-   _M_c = traits_type::eof();
  }
return __old;
   }
@@ -209,9 +208,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   _M_get() const
   {
const int_type __eof = traits_type::eof();
-   if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
-  _M_c = _M_sbuf->sgetc();
-   return _M_c;
+   return _M_sbuf ? _M_sbuf->sgetc() : __eof;
   }
 
   bool
@@ -418,7 +415,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  else
__c = __sb->snextc();
}
- __first._M_c = __c;
}
   return __first;
 }
-- 
2.10.1