Re: [boost] [afio] Formal review of Boost.AFIO

Subject: Re: [boost] [afio] Formal review of Boost.AFIO
From: Andreas Schäfer (gentryx_at_[hidden])
Date: 2015-08-30 20:33:39

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_programs/so_what.html
[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
[phone]
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!