|
|
Boost : |
Subject: Re: [boost] [afio] Formal review of Boost.AFIO
From: Thomas Heller (thom.heller_at_[hidden])
Date: 2015-08-24 07:33:21
Hello,
This is my review of AFIO.
DISCLAIMER: I am not on a personal vendetta or hold any grudge against
the author. I spent valuable family time in order to review the
presented work and opinions expressed so far.
On 08/23/2015 03:08 AM, Ahmed Charles wrote:
> Please answer the following questions:
>
> 1. Should Boost.AFIO be accepted into Boost? Please state all
> conditions for acceptance explicitly.
I don't think it should be accepted as a Boost Library as it currently
stands. The documentation presented is incomplete and tries to sell
features which are only to be finished eventually. Furthermore, I am not
exactly sure about the scope of the library and the final goal.
For example:
- The author clearly states that performance is not one of his goals
(http://thread.gmane.org/gmane.comp.lib.boost.devel/261914/focus=262478). Yet,
the documentation, especially the introduction, puts a great emphasize
on that, while the presented benchmarks do not show any of those
promises or compare apples with oranges.
- Portability, the author claims to present a portable solution to the
ACID problem however goes into great length in explaining the
differences between various OSes and FSes from which I concluded that
users of the library still have to implement special code paths for some
systems. This comes especially obvious when looking at example code
provided in the documentation.
- "Readiness": The docs list tons and tons of features which aren't
really there yet. One can get the impression that it is only half done.
Especially concerning is that running the unit tests might actually
break your hard drive
(https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/extents_theory/extents.html)
which is not very assuring in the context of the promised feature set.
It looks like it would have been better to let the dust settle after the
final additions to the documentation and maybe even properly implement
the praised backend before aiming for review. I think it would also be
best to only include the latest version of the library for inclusion to
boost, is the community really supposed to also review the apparently
inferior V1 of the library?
> 2. What is your evaluation of the design?
The design of the documented, user facing API isn't what I expected from
a modern C++ API. Following are some points, in no particular ordering,
that struck me most.
- afio::future<T = void>:
As far as I can see, there is not a single function documented that
returns anything than some equivalent to "shared_future<handle_ptr>".
The usage of afio::future<void> (explicitly added the default parameter)
is, IMHO, very confusing as soon as you pass them as dependencies to
another operation. The void template parameter always looks like "just a
signal" instead most of the time, the actual *handle* associated with
that future is the dependency, and from time to time even needed as an
argument to the async operation. In addition,
"shared_future<handle_ptr>" has two indirections, one to the shared
state of the future and one to the actual handle.
Moreover, I disagree that afio::future<T> should have shared future
semantics by default. I am a proponent of the
std::future/std::shared_future design and believe this should be
followed by all libraries dealing with some sort of futures. I think
most of the design decisions are based on shared_future only semantics,
which is probably the reason why I have problems with them.
- Extensive use of shared_ptr:
Some functions return some shared_ptr (specifically handle and
dispatcher), therefore probably always relying on some sort of memory
allocation. This seems to be due to the fact that only opaque abstract
base classes are returned from those API functions. Why not return
unique_ptr or even by value of the distinct type?
> 3. What is your evaluation of the implementation?
I refrained from actually looking at the implementation. This is
actually a pretty severe point. The meat of the implementation is split
into 4 headers only, with two headers (afio.hpp and
detail/impl/afio.ipp) having the majority code with over 6000 lines of
codes each! Despite the actual content, I believe this is a maintenance
nightmare. I don't want to review such large blobs of text.
> 4. What is your evaluation of the documentation?
The documentation is very hard to follow and incomplete. The layout of
the table of contents as well as the enumerations documentation is
broken. Some examples in the reference documentation do not actually
match the presented API. Some examples contain errors.
On the hard to follow part:
- Overall, the documentation is cluttered with features that are going
to be implemented in future versions or even claim that the features
already exist.
- I have no idea what the table in
https://boostgsoc13.github.io/boost.afio/doc/html/afio/overview.html is
trying to tell me
- The design introduction contains lots of text that seems like
unrelated anecdotes
- The reference section doesn't cross link between the different
related API functions/classes that are needed for one specific
class/function.
On the incomplete part:
- The workshop tutorial is not finished.
- Some references are not completely documented, for example:
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/classes/directory_entry.html
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/classes/dispatcher.html
On the examples do not match reference documentation part:
-
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/functions/make_io_req_3_length_deducing.html
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/functions/make_io_req_4_length_specifying.html
The example mentioned in the above entries do not mention the
make_io_req function at all.
-
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/functions/dir/dir_1_batch.html
Why does this example not focus on the presented function?
On the examples containing errors part:
-
https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/naive_afio_async.html
"
// Boost.Monad futures are also monads, so this implies a
make_ready_future()
return std::current_exception();
"
The return type is shared_future<data_store::ostream>. What does this
have to do with Boost.Monad and any of its behavior? Which shared_future
is meant here that has a non-explicit ctor
that takes a std::exception_ptr?
- For example (there are many more):
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/functions/file/file_1_batch.html
documents dispatcher::file taking a vector of path_req, in the
example, the vector is constructed, but only the front is passed to the
function. This seems very odd.
Other documentation flaws:
-
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/functions/process_threadpool.html
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/macros_flags/boost_afio_validate_inputs.html
looks odd, there are no parameters or anything there, so why does this
page contain a section for it?
-
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/macros_flags/file_flags.html
the synopsis is very hard to read. (same goes for:
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/macros_flags/fs_metadata_flags.html
and
https://boostgsoc13.github.io/boost.afio/doc/html/afio/reference/macros_flags/metadata_flags.html)
- Why document undocumented features?
https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/mini_programs/atomic_logging.html
- Can you please strip example code from commented sections of code
that obviously don't add anything to the example (or "#if 0"ed sections
or "legacy" behavior)?
- It would have been nice to see all examples (at least in a non
advanced section) to not rely on platform dependent macro definitions.
In addition, I personally dislike the "Try AFIO now in online web
compiler" button. Why does it have to appear on *every* single page
(there is also a bug that it doesn't appear in some of the reference doc
sections)? Besides, it doesn't really match the style of the rest of the
documentation. Which brings me to the "Comments" part, doesn't match the
style either, which makes it look totally out of place especially since
it comes from some third party provider, and I am not sure I can trust
that. It looks especially odd when javascript is disabled. Isn't
discussing a library and its documentation on a mailing list not enough?
> 5. What is your evaluation of the potential usefulness of the library?
I think having a library which handles asynchronous file I/O operations
is potentially very useful. I like the idea and can see potential
benefits over traditional synchronous file I/O. Mostly in terms of
overlapping I/O with other useful work. I am not sure how asynchrony
helps with solving the ACID problem as I am not too versed in that
domain and the documentation doesn't make that clear either.
> 6. Did you try to use the library? With what compiler? Did you have
> any problems?
Haven't tried the library.
> 7. How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
I followed the email threads that had been ongoing around the time of
this writing and tried to follow the documentation, which took me about
4 hours.
> 8. Are you knowledgeable about the problem domain?
I am not knowledgeable about async file I/O in particular but spent most
of my time with dealing with an async based parallel runtime system and
its application.
Best regards,
Thomas
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk