|
Affects all released versions.
Let's try to look into speeding this up in 4.2.1. All of the above testcases are testing both the time taken by getline() and the time to create and setup a file stream. The following testcase tests just the time it takes to do the getline, everything else should be constant.
#include <iostream>
#include <fstream>
#include <string>
#include <stdlib.h>
int main (int argc, char* argv[])
{
size_t loops = 1000;
if (1 < argc)
loops = strtoul (argv [1], 0, 0);
const char* file = __FILE__;
if (2 < argc)
file = argv [2];
std::ifstream ifs (file);
if (!ifs.is_open ())
return 1;
size_t lines = 0;
std::string line;
for (size_t i = 0; i < loops; ++i)
{
while (std::getline (ifs, line, '\n'))
++lines;
ifs.clear ();
ifs.seekg (0);
}
std::cout << lines << std::endl;
return 0;
}
The issue is that the std::basic_string<...>::erase() method deallocates the string body, and then immediately reallocates a new one when we call append(). Obviously this is an unnecessary cost, and it can be quite expensive.
The std::basic_string<...>::clear(), erase() and replace() methods all have this behavior when resizing down to a zero length string. I'm inclined to believe that this is a simple mistake, but it is possible that it was done intentionally so that strings are deallocated when they become empty. My intention is to modify clear() and replace() to only deallocate the body when actually necessary. This will have the side effect that a string that has its size reduced to 0 will continue to hold the memory that it used prior to that resize. If a user wishes to deallocate this memory, they will have to use the following.. // swap implementations with a null string to deallocate memory std::string ().swap (s); Another option is to leave the current behavior for erase() and replace(), and to fix the issue for clear() [or vice-versa]. Here are the results of running the test before any changes... $ time ./t-gcc 1000 /nfs/devco/vitek/stdcxx/trunk/tests/strings/21.string.io.cpp 1291000 real 0m0.587s user 0m0.563s sys 0m0.021s $ time ./t-stdcxx 1000 /nfs/devco/vitek/stdcxx/trunk/tests/strings/21.string.io.cpp 1291000 real 0m1.022s user 0m0.907s sys 0m0.068s Here is the result after the patch... $ time ./t-stdcxx 1000 /nfs/devco/vitek/stdcxx/trunk/tests/strings/21.string.io.cpp 1291000 real 0m0.309s user 0m0.226s sys 0m0.081s Here is the changelog 2008-01-29 Travis Vitek <vitek@roguewave.com> STDCXX-231 * include/string (clear): Avoid deallocating string body unless necessary. * include/string.cc (replace): Ditto. * include/istream.cc (getline): Call clear() instead of erase() to avoid unnecessary overhead. Prevented the Wiki plugin from formatting the Description.
Wow, that's quite an improvement! IMO, not deallocating string memory on clear() or erase() makes perfect sense. I'm just not completely sure that it isn't too big a change for a micro release. If there is code out there that's optimized for space and that assumes these functions have deallocating semantics we might be changing its performance characteristics in an unexpected way. I might feel more comfortable if I knew that most other implementations did deallocate... Then again, maybe I'm being overly cautious...
This is the Dinkum implemenation on MSVC 8.0...
C:\>type t.cpp && cl /nologo /EHsc t.cpp && t.exe
#include <string>
#include <iostream>
int main ()
{
std::string s (1000, 'a');
std::cout << s.capacity () << std::endl;
s.clear ();
std::cout << s.capacity () << std::endl;
s.erase ();
std::cout << s.capacity () << std::endl;
s.replace (0, s.size (), "1", 0);
std::cout << s.capacity () << std::endl;
std::string ().swap (s);
std::cout << s.capacity () << std::endl;
return 0;
}
t.cpp
1007
1007
1007
1007
15
And this is the result with the GNU implementation... $ g++ t.cpp && a.out 1095 1095 1095 1095 0 Fix format so that page doesn't run over.
Thanks for the numbers! I'm guessing the capacity doesn't drop below 15 in the Dinkumware string because they implement the small string optimization.
So as far as fixing this in 4.2.1 or 4.3, what do you propose? Well, this fix is functionally incompatible. According to our compatibility guidelines
 , this makes it binary incompatible, so it would not be suitable for 4.2.1. Personally, I'd like to commit it for 4.2.1, but that would be against the rules.
So for now it looks like the right thing to do is to submit it to trunk and just not merge it out to 4.2.x. Committed to trunk in r617251 Verified in nightly testing.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
std::getline from <string> header is rather slow
$ ls -l input.txt && time LD_LIBRARY_PATH=../lib ./getline 1 input.txt && time ./a.out 1 input.txt && time wc -l input.txt && cat getline.cpp
rw-rw-r- 1 mbrown mbrown 32942720 Oct 23 11:30 input.txt892370
real 0m0.433s
user 0m0.400s
sys 0m0.036s
892370
real 0m0.149s
user 0m0.124s
sys 0m0.024s
892370 input.txt
real 0m0.053s
user 0m0.048s
sys 0m0.004s
#include <stdio.h>
#include <stdlib.h>
#include <fstream>
#include <string>
int main (int argc, char *argv[])
{
unsigned long loops;
const char* file;
loops = 1 < argc ? strtoul (argv [1], 0, 0) : 1;
file = 2 < argc ? argv [2] : _FILE_;
unsigned long line_count = 0;
std::string line;
for (unsigned long i = 0; i != loops; ++i) { std::ifstream in (file); if (!in.is_open ()) return 1; while (std::getline (in, line)) ++line_count; }
printf ("%lu\n", line_count);
}