Hi, this is my review of Boost.AFIO. Having read a good deal of the mails in this thread, I'd like to assure everyone that I'm not on a personal vendetta. I'm also married and have to wonderful kids and I'm not being paid for this review. All of which is totally irrelevant here. I try to keep this short by trying not to repeat what others have said before ("backwards compatibility cruft, shared state of futures...) On 18:08 Sat 22 Aug , Ahmed Charles wrote:
1. Should Boost.AFIO be accepted into Boost? Please state all conditions for acceptance explicity.
I vote no. For acceptance I'd expect a library that outperforms "naive" OpenMP code. Also, the design should be greatly simplified. The current implementation does not meet my expectations WRT compatibility. See below for explanations.
2. What is your evaluation of the design?
The design is overly complicated. For instance a whole stand-alone library "Boost.Monad" is pulled in which is designed so that numerous different "future-ish" types can be created through it. Yet, only one type is apparently being used in Boost.AFIO (according to the author[1]). Dropping that (currently) unnecessary flexibility would simplify the design.
3. What is your evaluation of the implementation?
I tried to assess the library's performance as IMHO (and the library's documentation) performance is the major motivation for using AFIO. The code failed to compile with Intel's C++ compiler icpc[2]. With g++ 4.9.3 the code compiled, but segfaulted when traversing a Linux 4.1.6 source tree[3]. The code (and the naive OpenMP implementation provided in the documentation) completed successfully on a Boost.AFIO source tree, but the OpenMP code (~600 MB/s) was significantly faster than the AFIO code (~270 MB/s), see [4]. Test system: Intel Core i7-3610QM, Samsung EVO 1TB SSD, Linux 4.1.5. (I can provide further details if necessary.) All of this does not positively impress me.
4. What is your evaluation of the documentation?
I was only looking at the code samples. These are nicely documented. For some reason include statements were left out, despite some of them exceeding 200 LOC. They include inactive passages ("#if 0") which, together with the length, impair legibility.
5. What is your evaluation of the potential usefulness of the library?
I came here because I thought portable asynchronous file IO is an important, currently unsolved problem. I think such a library could be of tremendous use.
6. Did you try to use the library? With what compiler? Did you have any problems?
I tried icpc 15.0.3 (failure, as shown above) and g++ 4.9.3 (success)
7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I did study the benchmark example and took a deeper dive into the design of monad and AFIO's future type. A tad more than 10h in total (including this write up).
8. Are you knowledgeable about the problem domain?
I have >10 years of experience with performance tuning and scalable IO (mostly MPI-IO). This is perhaps not the exact same problem domain, but parallels exist. Concluding remarks: I found this review confusing. 1. The documentation claims "I invested two days into achieving maximum possible performance as a demonstration of AFIO's power"[5], yet the author states "Is it [AFIO] lower performance than alternatives? Almost certainly[...]"[6]. 2. On the one hand, the author writes "The library being presented today is very well tested" and "I'm a perfectionist. I wouldn't have considered it ready for review until it was all green across the board on all 60 of its per-commit tested targets. I spent four hours on Friday coaxing wandbox to compile AFIO again so you'd have a working C++ browser playpen you can go play with AFIO in. Little details are important.", yet the code failed in my very basic tests. 3. The author claims high code stability for the current implementation ("The core engine has been stable since 2013/2014. How much more stable does a Boost library need to be before being considered production ready?"), yet none of that code will be there for long, as the author writes "I spent last week mocking up the v1.4 AFIO API based on wrappers and shims atop the v1.3 engine as a prelude to rewriting the engine completely based on lightweight monads + future promise". A complete engine rewrite! A new API! 4. I'll let this one stand for itself: "I am estimating four months to do the rewrite and debug of the engine, and I usually underestimate." vs. "[...]which is my current estimate, and I am usually within 10% of my estimates when estimating on my own code" 5. All this noise on feuds, bullying, late night hacking sessions, 50h work weeks, valuable/lost family time, lack of fitness... what is it supposed to mean? Should I now worry about someones kids not being with their father because I submitted an AFIO bug report? Or will I be accused of a personal vendetta because my critique was not received as being friendly enough? Others have said this before, and, since these tendencies returned just today, I'll repeat: let's keep this professional. Keep personal sensitivities out of this discussion. Convince with technical brilliance, not temperamental brittleness. Cheers -Andreas [1] http://lists.boost.org/Archives/boost/2015/08/224901.php [2] https://gist.github.com/gentryx/5ebbc48b1a93175b24a9 [3] https://gist.github.com/gentryx/09376dc8de569eb52e88 [4] https://gist.github.com/gentryx/7053b2a576ee8410be75 [5] https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/mini_progr... [6] http://lists.boost.org/Archives/boost/2015/08/224788.php -- ========================================================== Andreas Schäfer HPC and Grid Computing Department of Computer Science 3 Friedrich-Alexander-Universität Erlangen-Nürnberg, Germany +49 9131 85-27910 PGP/GPG key via keyserver http://www.libgeodecomp.org ========================================================== (\___/) (+'.'+) (")_(") This is Bunny. Copy and paste Bunny into your signature to help him gain world domination!