[outcome] Andrzej's review

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[outcome] Andrzej's review

Boost - Dev mailing list
Hi Everyone,
This is my review of Boost.Outcome v2. The library should be *accepted*
subject to conditions that I list down below.


What is your evaluation of the design?


Clean and logically divided.

What is your evaluation of the implementation?
>

Some bugs aside, it is clean and well structured. I was able to read it and
understand how things work. I will be filing bugs in GitHub.

What is your evaluation of the documentation?
>

Good. You can get the idea what the library is doing and how you can use
it. But I should mention that I wrote some pieces thereof, so my opinion is
likely biased. The docs need some modifications before they can get into
Boost. I mean macro names, and the technology required to build it.

What is your evaluation of the potential usefulness of the library?
>

I find the library very useful in certain domains. I myself have found two
applications in my job project, and they are not really related to
light-speed efficiency:

1. We have a task scheduling framework built atop ASIO, and signalling
failures through exceptions makes the code really difficult to read because
try-catch blocks need to work as control statements. Also, we cannot pass
them by exception_ptr as sometimes we need to classify the kind of failure
to determine if we want to deal with it in the framework, or if we want to
forward them to the user.

2. I have a validation routine that validates (and parses) the input data
written using some syntax. I wan the syntax-errors in input files to be
forwarded as failed `result`s (no exceptional about these), but other
errors like memory shortage during the file parsing to be signaled via
exceptions (and I will probably kill the module in response).

Why accepting this library when we have `expected` being standardized? I do
not really mind having two in Boost. First, it is ready, being proposed,
and applied in a number of non-trivial code-bases. Niall claims that his
trade-offs are better tailored to predictable-latency applications. I do
not have enough knowledge to asses it. They look convincing. The only way
to test it is to give this library the Boost blessing and have the user
test it. I am bought by the TRY operations, and the upgrade to `outcome`
looks really convincing.

Did you try to use the library? With what compiler? Did you have any
> problems?
>

Yes. A lot of compilers. For instance GCC 6.4.0, also different GCC and
Clang versions in WandBox. No problems encountered.

How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
>

I have read most of the final implementation. I have read many versions of
the documentation. I have spent a lot of time (prior to the review) asking
Niall lots of questions about the details. I have also built lots of small
examples to test the library and some corner cases.

Are you knowledgeable about the problem domain?
>

I do report failures (by different means) in my programs. I have given it
quite some thought. I am well familiar with implementation and formal
specification of `boost::optional` and `std::optional` which had to address
some similar implementation problems.

Do you think the library should be accepted as a Boost library?
>

Yes, conditionally. Here are three conditions:

1. Documentation is adjusted to use boost-standard names of macros and
namespaces. Also, it is not clear if Boost release process is capable of
handling the technology that Niall has chosen. If not, the contents of
documentation should be migrated to another format, manageable by Boost
release process.

2. Because there has been some confusion around the meaning of storing
default-constructed `std::error_code` (is it an error or lack thereof?) and
`std::exception_ptr` (e.g., see this issue:
https://github.com/ned14/outcome/issues/115), I request that you state in
the docs, before the API Reference how library treats default-constructed
values of `EC` and `EP`. For instance, you can say "default-constructed
error_code is considered an error, and treated as such by the library", or
"the library sometimes may make use of the fact that a default-constructed
EC/EP is not an error, so make sure not to pass us such values": I am fine
with any answer, but I need you to spell it out, so that I know what to
expect.

3. Similarly, I request that you put in the docs what is the "value" of an
object of type `outcome<>` or `result<>` in the sense in which John Lakos
talks about "salient attributes".
I know that T, EC, and EP constitute the value. But in the middle of the
tutorial we learn that apart from these you also store "spare_storage" --
is it also included in copying and comparisons? API reference on
`operator==` only says "compares if two objects are equal", but it is not
that clear what it means. Also, what does it mean that result is equal to
optional. Define it somewhere and paste links to it.

Finally, I wan to thank Niall for his now many-year effort to deliver this
really cool library to the community.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [outcome] Andrzej's review

Boost - Dev mailing list
On Fri, Jan 26, 2018 at 10:17 AM, Andrzej Krzemienski via Boost
<[hidden email]> wrote:
> The only way to test it is to give this library the Boost blessing and have
> the user test it. I am bought by the TRY operations, and the upgrade to
> `outcome` looks really convincing.

Acceptance into Boost should not be used to "give users a library to
test." Motivated users are more than capable of using libraries that
are not already in Boost (for example, Beast already had a nicely
sized following before being accepted). This sounds a lot like the bad
practice of rushing libraries into the standard. Or another variation
of "we have to pass the bill to find out what's in it."

A library that is good will find users adopting it regardless of
whether the name "Boost" appears in front of it or not.

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [outcome] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Fri, Jan 26, 2018 at 10:17 AM, Andrzej Krzemienski via Boost <
[hidden email]> wrote:

> Why accepting this library when we have `expected` being standardized? I do
> not really mind having two in Boost.


I agree, we should not be rejecting libraries just because another similar
library already exists in Boost.


> First, it is ready, being proposed,
> and applied in a number of non-trivial code-bases. Niall claims that his
> trade-offs are better tailored to predictable-latency applications. I do
> not have enough knowledge to asses it. They look convincing. The only way
> to test it is to give this library the Boost blessing and have the user
> test it.


Here I disagree. If there are issues with a library, they should be
addressed before it is accepted; we should be proposing/accepting only
relatively mature libraries that have seen real, if limited, deployment.
First, this is a matter of quality standards. Secondly, fixing problems
when there already is substantial user base is messy, and often requires
further compromise.

Emil

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [outcome] Andrzej's review

Boost - Dev mailing list
>> Why accepting this library when we have `expected` being standardized? I do
>> not really mind having two in Boost.
>
> I agree, we should not be rejecting libraries just because another similar
> library already exists in Boost.

There is no similar library already in Boost. Expected will not be
submitted to Boost, it's in LWG now.

I believe Andrzej meant "second" or "alternative", not "two".

>> First, it is ready, being proposed,
>> and applied in a number of non-trivial code-bases. Niall claims that his
>> trade-offs are better tailored to predictable-latency applications. I do
>> not have enough knowledge to asses it. They look convincing. The only way
>> to test it is to give this library the Boost blessing and have the user
>> test it.
>
> Here I disagree. If there are issues with a library, they should be
> addressed before it is accepted; we should be proposing/accepting only
> relatively mature libraries that have seen real, if limited, deployment.

At a minimum, Outcome v2 is already in production use at Microsoft and
at Google. I have received bugs reports :) I suspect a few other
multinationals are also using it in production, but I cannot be sure.

> First, this is a matter of quality standards. Secondly, fixing problems
> when there already is substantial user base is messy, and often requires
> further compromise.

I believe Andrzej was specifically referring to the fixed latency use
case only.

On that the only hard evidence I can provide is AFIO benchmarking on
Intel Optane technology. AFIO, which uses Outcome throughout, benchmarks
at a minimum of 100 nanosecond i/o latency on Linux (pre Meltdown
mitigations). This is statistically indistinguishable, to a 99%
confidence interval, to calling the kernel syscall directly.

How useful Outcome is for fixed latency programming below 100
nanoseconds per operation I cannot say with any statistical confidence.
Also, other workload patterns may differ significantly to kernel i/o.

In the end, people ought to give Outcome a try for their use case and
see how it goes for them. Which is what Andrzej was saying I believe.

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [outcome] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
>> The only way to test it is to give this library the Boost blessing and have
>> the user test it. I am bought by the TRY operations, and the upgrade to
>> `outcome` looks really convincing.
>
> Acceptance into Boost should not be used to "give users a library to
> test."

I believe Andrzej was referring only to the fixed latency use case. That
is, at least, how I read his words given the context at the point of use.

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [outcome] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> How much effort did you put into your evaluation? A glance? A quick
>> reading? In-depth study?
>
> I have read most of the final implementation. I have read many
> versions of the documentation. I have spent a lot of time (prior to
> the review) asking Niall lots of questions about the details. I have
> also built lots of small examples to test the library and some corner
> cases.

For the purposes of proper disclosure, I personally would consider
Andrzej a coauthor of v2 Outcome. He didn't write any code, but he was
my auditor throughout v2's development, watching new code land and
telling me all sorts of problems with it, usually through asking really
useful stupid questions which provoked me to think again. He upped my
game considerably, and I'd like to thank him once again for that, and
for all the new just submitted detailed bug reports. I shall attend to
them next week once this weekend's usual childcare is complete.

> Yes, conditionally. Here are three conditions:
>
> 1. Documentation is adjusted to use boost-standard names of macros
> and namespaces. Also, it is not clear if Boost release process is
> capable of handling the technology that Niall has chosen. If not, the
> contents of documentation should be migrated to another format,
> manageable by Boost release process.

No problem. I have a script here already which converts the Markdown. If
accepted, I am happy to work with boost-website's maintainers to figure
out some formulation of tooling everyone is happy with.

> 2. Because there has been some confusion around the meaning of
> storing default-constructed `std::error_code` (is it an error or lack
> thereof?) and `std::exception_ptr` (e.g., see this issue:
> https://github.com/ned14/outcome/issues/115), I request that you
> state in the docs, before the API Reference how library treats
> default-constructed values of `EC` and `EP`.

I am happy to spell out that the library makes no special interpretation
of `EC`'s or `EP`'s value at all.

For example, if `EC` matches `trait::has_error_code_v<EC>` in a
`result`, then `error_code_throw_as_system_error` policy is selected as
a default (https://ned14.github.io/outcome/tutorial/policies/builtin/).
If one then observes `.value()` when there is no value, the policy will
`OUTCOME_THROW_EXCEPTION(std::system_error(error))` or
`OUTCOME_THROW_EXCEPTION(std::system_error(make_error_code(error)))` as
appropriate.

I do not understand the differentiation of default construction over any
other construction. You can absolutely construct a `result<T>` using a
default constructed `error_code`, and upon `.value()`,
`OUTCOME_THROW_EXCEPTION(std::system_error(error_code()))` would occur.
Which would not be particularly useful, but then Outcome makes no
special interpretation of `EC`'s value at all. Just its type is to
activate traits (or not).

> For instance, you can say "default-constructed error_code is
> considered an error, and treated as such by the library", or "the
> library sometimes may make use of the fact that a
> default-constructed EC/EP is not an error, so make sure not to pass
> us such values": I am fine with any answer, but I need you to spell
> it out, so that I know what to expect.

Can you show me the exact page where I say the second sentence? Maybe
it's because it's 1:50 am and I'm too tired after a 6am start, but I
cannot find that sentence in the documentation.

> 3. Similarly, I request that you put in the docs what is the "value"
> of an object of type `outcome<>` or `result<>` in the sense in which
> John Lakos talks about "salient attributes". I know that T, EC, and
> EP constitute the value. But in the middle of the tutorial we learn
> that apart from these you also store "spare_storage" -- is it also
> included in copying and comparisons?

Yes in copying where copying is not explicitly done (i.e. type
conversion is performed).

No in comparisons.

I will add this to the documentation. Tracked at
https://github.com/ned14/outcome/issues/117

> Also, what does it mean that result is equal to
> optional. Define it somewhere and paste links to it.

Do you mean `optional<T>` being equal to `result<T, void>` or something?

Or do you mean via the `ValueOrNone` concept, or something else entirely?

> Finally, I wan to thank Niall for his now many-year effort to deliver
> this really cool library to the community.

Thank you for your review. You're right it's been a long road. Outcome
began life in 2014. I can't believe four years have passed on such a
small Result-ish type. But equally I don't think it could have been done
right any quicker: Outcome is not Expected because of the first Boost
peer review, that huge conversation just eight months ago led us to here
and now.

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [outcome] Andrzej's review

Boost - Dev mailing list
2018-01-27 2:54 GMT+01:00 Niall Douglas via Boost <[hidden email]>:

> > For instance, you can say "default-constructed error_code is
> > considered an error, and treated as such by the library", or "the
> > library sometimes may make use of the fact that a
> > default-constructed EC/EP is not an error, so make sure not to pass
> > us such values": I am fine with any answer, but I need you to spell
> > it out, so that I know what to expect.
>
> Can you show me the exact page where I say the second sentence? Maybe
> it's because it's 1:50 am and I'm too tired after a 6am start, but I
> cannot find that sentence in the documentation.
>

You do not say it in the docs. But one could get that impression. E.g. from
the previous versions, where function .error() default-constructed EC if it
was absent, and it was recommended to use .error() to check both existence
of error and its value.
Also, I got confused with the consequences of this bug:

```
#include "outcome.hpp"
namespace out = OUTCOME_V2_NAMESPACE;

int main()
{
  out::outcome<int> o {std::error_code{}};
  assert (o.has_error());
  out::outcome<int> p = o.as_failure();
  assert (!p.has_error());
}
```

Somewhere in the interaction between outcome and failure_type there is a
bug that treats default-constructed EC as no EC. And now I wonder: is it a
bug, or is it my fault that I used default-constructed code.


> > 3. Similarly, I request that you put in the docs what is the "value"
> > of an object of type `outcome<>` or `result<>` in the sense in which
> > John Lakos talks about "salient attributes". I know that T, EC, and
> > EP constitute the value. But in the middle of the tutorial we learn
> > that apart from these you also store "spare_storage" -- is it also
> > included in copying and comparisons?
>
> Yes in copying where copying is not explicitly done (i.e. type
> conversion is performed).
>
> No in comparisons.
>
> I will add this to the documentation. Tracked at
> https://github.com/ned14/outcome/issues/117
>
> > Also, what does it mean that result is equal to
> > optional. Define it somewhere and paste links to it.
>
> Do you mean `optional<T>` being equal to `result<T, void>` or something?
>
> Or do you mean via the `ValueOrNone` concept, or something else entirely?
>

Sorry, I am recovering from flu, and sometimes some bug sneaks in. I meant
comparing an `outcome<>` with a `result<>`. What are the semantics then?

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [outcome] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2018-01-26 20:59 GMT+01:00 Emil Dotchevski via Boost <[hidden email]>
:

> On Fri, Jan 26, 2018 at 10:17 AM, Andrzej Krzemienski via Boost <
> [hidden email]> wrote:
>
> > Why accepting this library when we have `expected` being standardized? I
> do
> > not really mind having two in Boost.
>
>
> I agree, we should not be rejecting libraries just because another similar
> library already exists in Boost.
>
>
> > First, it is ready, being proposed,
> > and applied in a number of non-trivial code-bases. Niall claims that his
> > trade-offs are better tailored to predictable-latency applications. I do
> > not have enough knowledge to asses it. They look convincing. The only way
> > to test it is to give this library the Boost blessing and have the user
> > test it.
>
>
> Here I disagree. If there are issues with a library, they should be
> addressed before it is accepted; we should be proposing/accepting only
> relatively mature libraries that have seen real, if limited, deployment.
> First, this is a matter of quality standards. Secondly, fixing problems
> when there already is substantial user base is messy, and often requires
> further compromise.
>

I am sorry if this sounded this way. I did not mean to say "add it to Boost
so that the users can check if it is fine". I consider the library design
correct and sound. Also the implementation (modulo bugs - but they are
trivial to fix in my judgement). However, I believe that there are some
things that we cannot judge by inspecting the code, the docs, the design:
that is, if it will prove convenient in practice. To me `std::expected`
looked reasonable and correct. But it is not as "usable" due to the lack of
TRY operations and upgrade to outcome. But no-one could invent them by only
reviewing the library and discussing it. It required field experience. And
now we have something convincing and seemingly practical. But it will
require a vaster experience to check if there aren't any unforeseen
shortcomings or better usage patterns.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost