Re: libstdc++/4150: catastrophic performance decrease in C++ code

[Loren James Rittle <rittle at latour dot rsch dot comm dot mot dot com>]

Loren>>> Having attempted patches similar to your proposal, I think
Loren>>> the hammer breaks those test cases that exercise interleaving
Loren>>> between C and C++ IO.

Hi again Jason,

Jason>> I don't think so.  If we're synced to stdio, we get buffers of
Jason>> size 1, so it doesn't matter which end we're calculating from.

This would seem true but there is a subtle issue related to whether a
character resides in the buffer or not.  A measurement from the point
at which characters have been consumed from stdio will *sometimes* be
off-by-one compared to how measurements were done before your patch.
If it was always off-by-one or off-by-buffer-size, then the exact case
detailed below wouldn't break.

Under the current architecture (which I have only ever tweaked for
performance, compliance and QoS of interactive cases not dictated by
standard), the whole reason for the backup to the point before the
read is that until a character is actually consumed by the
higher-layer of libstdc++-v3 IO, the lower-layer C stdio file-pointer
must not appear to move forward w.r.t. other C stdio.  Granted it
seems less-than-ideal to always use that algorithm even when not
sync'd to stdio.

Jason, this is why I thought your new code path is good for unsync'd
cases yet bad for sync'd cases.

It is why I told RTH the other day in an e-mail that I thought some
basic re-architecture would be required to solve all performance
issues related to outstanding libstdc++-v3 PRs.  When the higher-layer
knows it will consume more than X characters in sync'd IO cases, it
should be able to pull >1&<X characters from the lower-layer (current
architecture limits us to pulls of 1 character).  Or, if the
higher-layer knows it is looking for a newline character (another very
common case), it should be able to use the C stdio optimized routine
to pull >1 character from the lower layer (bounded only by newline or
the provided buffer size, aka the fgets function call).  Under a
re-architecture, it seems to me that only when the higher-layer of
libstdc++-v3 is in a scanning mode not directly supported by libc that
it must conditionally pull 1 character at a time through the layer
when sync'd to stdio (the bizarre read, backup pattern at the core of
the buffer refill routine that Jason and RTH rediscovered in the last
few days).  Now, I actually have no idea if the abstraction layer
dictated by the standard even allows these optimizations.  I looked at
this situation >6 months ago and I actually think not.

Loren> OK, cool.  I am playing with your exact patch for a while.  After
Loren> rebuilding libstdc++-v3 with it and ensuring no regressions on any
Loren> platforms I can personally check; I will look at the interactive
Loren> stream cases that have been posted over the years.

With your patch (plus the removal of the related _GLIBCPP_AVOID_FSEEK
region in src/ios.cc), I see one automatic regression here:

assertion "(off_2 == (off_1 + 2 + 1 + 1))" failed: file
"[...]/27_io/filebuf_virtuals.cc", line 428
FAIL: 27_io/filebuf_virtuals.cc execution test

I would think this would be seen in your environment but perhaps not.
Understandable, since the change in file position is not accounted
for.  The test looks at a subtle situation involving the file pointer.

Also, please disable test01 and enable test03 in main() of
27_io/narrow_stream_objects.cc and run it interactively.  It fails
with your patch in my environment.  The failure mode is attempting to
read too much data before responding.

Now I turn to performance improvements with your patch on
i386-unknown-freebsd4.5 (same hardware for all numbers).  IMHO, there
are some major per-system factors affecting performance of this test
case (or another important possibility that I will discuss below [1],
just for the record)...

Compiled with g++ 2.95.2 (used libstdc++-v2 with libio):

; time a.out log  #  51r    50.4u     0.1s       a.out log
; time a.out <log #  51r    50.4u     0.1s       a.out

Compiled with mainline without your patch (sync_with_stdio (false)
results similar for this port):

; time a.out log  #  65r    64.2u     0.1s       a.out log
; time a.out <log #  97r    95.4u     0.1s       a.out

Compiled with g++ mainline with your patch plus a removal of the
related _GLIBCPP_AVOID_FSEEK region in src/ios.cc:

; time a.out log  #  64r    63.4u     0.2s       a.out log
; time a.out <log #  89r    84.6u     0.0s       a.out

For the case that should be affected, about ~10% speedup in this
environment.

Adding sync_with_stdio(false) as RTH did to the test case:

; time a.out log  #  64r    63.3u     0.1s       a.out log
; time a.out <log #  64r    63.7u     0.1s       a.out

For the case that should be affected, about ~30% speedup in this
environment.

I am not against a 30% speedup in a common case, if all test suite
failures were resolved.  BTW, I am only taking Ben's role here since
he appears occupied with something else and I have already been
through this exercise with him more than once related to this area of
standard compliance verse performance.  I do agree that this is a flag
on the release.

Regards,
Loren

[1] I don't know if this is widely known information thus I want to
    make sure you tested my patch to enable _GLIBCPP_AVOID_FSEEK on
    Linux properly.  If you bootstrap all of gcc, then when
    libstdc++-v3 is built, it will be built with flags set by
    top-level Makefile (nominally, `-O2 -g').  If you later run make
    in libstdc++-v3, it will rebuild (some/all?) files with `-O0 -g'
    (except stuff built in libmath which appears to get top-level
    flags)...  IMHO, the only way to test performance patches in
    libstdc++-v3, is to `rm -rf <target>/libstdc++-v3' and rerun make
    at top-level.  This way libstdc++-v3 is built exactly as when it
    is bootstrapped.  BTW, when I rebuild libstdc++-v3 with
    `make clean all' instead, I see a performance slowdown in my
    environment similar to what you say you saw with my configuration
    patch on Linux:

    ; time a.out log  # 128r   126.8u     0.2s       a.out log
    ; time a.out <log # 291r   281.0u     0.5s       a.out

    The other explanation is that you tested my patch properly (anyone
    could get caught by this annoying trap thus I hope you took no
    offense to me walking through this issue completely): libc on
    FreeBSD has one performance profile with libstdc++-v3 and glibc on
    Linux has another (it would appear to be the cost of a getc/ungetc
    pair).  I don't use Linux but I do care to get libstdc++-v3
    working as well as g++ 2.95.2/libstdc++-v2 on all platforms.