Re: Review Request 34268: stout library - adding support for Solaris
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. 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 - Stan --- 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
Re: Review Request 34268: stout library - adding support for Solaris
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review85054 --- Ship it! Ship It! - Stan Teresen 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
Re: Review Request 34268: stout library - adding support for Solaris
On May 19, 2015, 12:17 a.m., Till Toenshoff wrote: Thanks a lot for this, Stan - much appreciated! There are a couple of style nits here and there and one basic question on the need of the `read`-variant for Solaris. For submitting an updated patch, please consult the patch submission guidelines http://mesos.apache.org/documentation/latest/submitting-a-patch/ specifically after Submit your patch - we need a patch that can be processed using our tooling and for that to work, an easy way is to follow that guide. Followed the guide and on git commit to the local branch I got a couple of more style recommendations which I fixed: git commit -m stout library - added support for Solaris Checking 5 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp:80: Tab found; better to use spaces [whitespace/tab] [1] 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp:110: Lines should be = 80 characters long [whitespace/line_length] [2] Total errors found: 2 But next step failed: $ support/post-reviews.py Running 'rbt post' across all of ... dbce4005a99e919fd0deaffda76e2e91fc63ade0 - (HEAD, solaris) stout library - added support for Solaris (10 minutes ago) Creating diff of: dbce4005a99e919fd0deaffda76e2e91fc63ade0 - (HEAD, solaris) stout library - added support for Solaris ... with parent diff created from: Press enter to continue or 'Ctrl-C' to skip. Failed to execute: 'rbt post --repository-url=git://git.apache.org/mesos.git --tracking-branch=master master dbce4005a99e919fd0deaffda76e2e91fc63ade0': CRITICAL: object of type 'NoneType' has no len() - Stan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review84202 --- On May 19, 2015, 12:21 a.m., Stan Teresen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 19, 2015, 12:21 a.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 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
Re: Review Request 34268: stout library - adding support for Solaris
On May 19, 2015, 12:17 a.m., Till Toenshoff wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp, line 72 https://reviews.apache.org/r/34268/diff/1/?file=961220#file961220line72 Could you please explain why the standard implementation of this function would not work for Solaris? getline is a GNU C library extension and not available by default on many Unix paltforms including Solaris. STL version proposed for Solaris can be used for everything as fully portable implementation unless there are use cases where performance can be a factor. - Stan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review84202 --- On May 19, 2015, 12:21 a.m., Stan Teresen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 19, 2015, 12:21 a.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 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
Re: Review Request 34268: stout library - adding support for Solaris
--- 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. Changes --- Uploaded a new patch which addressed the comments (post-review.py didn't fork for me) Repository: mesos-incubating Description --- stout library - adding support for Solaris Diffs (updated) - 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
Re: Review Request 34268: stout library - adding support for Solaris
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/#review84068 --- Ship it! Ship It! - Stan Teresen On May 15, 2015, 2:25 p.m., Stan Teresen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34268/ --- (Updated May 15, 2015, 2:25 p.m.) Review request for mesos. 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 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
