> On June 1, 2015, 10:36 a.m., Till Toenshoff wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp, lines 71-82 > > <https://reviews.apache.org/r/34268/diff/2/?file=969835#file969835line71> > > > > Seems we got some options here; > > A. use your separate, stream-based approach for solaris. > > B. use your separate, stream-based approach for all systems. > > C. re-implement getline within stout for solaris (e.g. > > http://opensource.apple.com/source/cvs/cvs-29/cvs/lib/getline.c) > > > > Option A. feels a bit weird as it presents a solution that should work > > on all systems, so why bother with alternatives - but see B :). > > > > Option B. Using streams has the nimbus of being slow - I have no prove > > at hand but that concern already got raised when I discussed your approach > > with a team-mate. > > > > Option C. Feels just right to me also because in the future, we may > > encounter more systems lacking of those GNU C extensions. > > > > What do you think, could we go for C. in your patch? We could also pick > > A. for now and add a comment (TODO) proposing Option C. to get implemented > > as soon as other systems with the lack of GNU C extensions are to be > > supported. > > Stan Teresen wrote: > My sipmle test program (which reads input file into std::string and > writes it to stdout with piping into /dev/null) showed that STL > implementation is about 30% slower than implementation which uses C getline > from the link you providied. Native library version is about 20 times faster > than both of them. These results are from run with ~7Mb input file. > For smaller input files (~200kb) the difference between first two > approaches is even smaller but the last one becomes just ~2 times faster. > > This, of cause, quite a limited test makes me think that for performance > reasons we might want to settle on two separate implementations - native one > for the systems with support and STL version for other systems as a simple, > concise, most portable and "C++ way" implementation > > So... option A in my opinion
Thanks for this quick micro benchmarking. I agree and think that this is perfectly fine for reasoning your approach. Dropping the issue. - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review85719 ----------------------------------------------------------- On May 22, 2015, 7:15 p.m., Stan Teresen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34268/ > ----------------------------------------------------------- > > (Updated May 22, 2015, 7:15 p.m.) > > > Review request for mesos, Joris Van Remoortere and Till Toenshoff. > > > Repository: mesos-incubating > > > Description > ------- > > stout library - adding support for Solaris > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp d2ca4be > 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 86949ec > 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp b0ed5cc > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp 81d64cc > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/34268/diff/ > > > Testing > ------- > > > File Attachments > ---------------- > > adding missing new file: stout/os/sunos.hpp > > https://reviews.apache.org/media/uploaded/files/2015/05/15/a2e296fa-e251-4467-9873-77d8ced7f0a3__sunos.hpp > > > Thanks, > > Stan Teresen > >