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/#review86172 --- Ship it! Ship It! - Till Toenshoff 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 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
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/#review84974 --- Patch looks great! Reviews applied: [34268] All tests passed. - Mesos ReviewBot 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/#review84202 --- 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. File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp https://reviews.apache.org//r/34268/#fcomment67 s/Linux/SunOS/ File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp https://reviews.apache.org//r/34268/#fcomment68 Not used?! File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp https://reviews.apache.org//r/34268/#fcomment69 Not used?! File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp https://reviews.apache.org//r/34268/#fcomment70 Please wrap to get below 80 chars per line. ``` TryDuration utime = Seconds(pstatus.pr_utime.tv_sec) + Nanoseconds(pstatus.pr_utime.tv_nsec); TryDuration stime = Seconds(pstatus.pr_stime.tv_sec) + Nanoseconds(pstatus.pr_stime.tv_nsec); ``` File Attachment: adding missing new file: stout/os/sunos.hpp - sunos.hpp https://reviews.apache.org//r/34268/#fcomment71 Please wrap to stay below 80 chars per line. Also when looking at this patch with an editor, I noticed that your intendtion is partially off here - we use soft-tabs, 2 spaces for all C++ source files. ``` return Process(pstatus.pr_pid, pstatus.pr_ppid, pstatus.pr_ppid, pstatus.pr_sid, None(), utime.isSome() ? utime.get() : OptionDuration::none(), stime.isSome() ? stime.get() : OptionDuration::none(), psinfo.pr_fname, (psinfo.pr_nzomb == 0) (psinfo.pr_nlwp == 0) (psinfo.pr_lwp.pr_lwpid == 0)); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp https://reviews.apache.org/r/34268/#comment135382 We commonly comment on the `#endif` from `#ifdef` `#endif` combinations quoting the clause. ``` #endif // NAME_MAX ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp https://reviews.apache.org/r/34268/#comment135381 ``` #endif // __sun ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp https://reviews.apache.org/r/34268/#comment135383 ``` #endif // __sun ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp https://reviews.apache.org/r/34268/#comment135378 Complete sentence with punctuation, please. ``` // FTS is not available on Solaris. ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp https://reviews.apache.org/r/34268/#comment135384 ``` #endif // __sun ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp https://reviews.apache.org/r/34268/#comment135371 Add a new line please. 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp https://reviews.apache.org/r/34268/#comment135372 Use complete sentences with punctuation please: ``` // Not defined on Solaris, taking a spare flag. ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp https://reviews.apache.org/r/34268/#comment135377 See below on `read`. 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp https://reviews.apache.org/r/34268/#comment135373 Could you please explain why the standard implementation of this function would not work for Solaris? 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp https://reviews.apache.org/r/34268/#comment135376 Insert new line, please. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp https://reviews.apache.org/r/34268/#comment135374 We do not commonly comment the `#endif` of a `#if define()` ``` #endif ``` Looking forward to give your updated patch another review, thanks again. - Till Toenshoff 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 -
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