Re: [boost] [compute] Review comments

Subject: Re: [boost] [compute] Review comments
From: Kyle Lutz (kyle.r.lutz_at_[hidden])
Date: 2014-12-28 19:42:16

On Sun, Dec 28, 2014 at 1:40 PM, Asbjørn <lordcrc_at_[hidden]> wrote:
> Hi,
>
> not a proper review, just some comments from a library user based on
> studying documentation and writing a couple of simple programs using
> Boost.Compute on Intel's and NVIDIA's OpenCL implementations (i7 CPU and
> GTX970 GPU respectively). I used MSVC2013 for the tests.
>
> Some of these comments I've shared previously on this list, but I'm
> repeating them here for completeness.
>
> I have some background in heterogeneous development and OpenCL in
> particular, but I'm no expert. I found Boost.Compute easy to pick up and
> providing a nice C++ layer, with a nice mix of low and high level.
>
>
> Comments in no particular order:
>
> 1) As a library user and one who's not a C++ guru by any means, the error
> messages thrown up when passing incorrect parameters can be quite confusing.
>
> For example, first time I tried to use Boost.Compute I managed to not pass
> the ouput iterator parameter to transform(), so that the lambda expression
> ended up in that position instead. This resulted in several very confusing
> errors inside result_of.hpp, with no errors anywhere near my or
> Boost.Compute source code.
>
> It would be nice if the compilation could fail in more informative manners.

Fully agree. There are many places where more static_assert's could be
added to help improve compile error messages. I'll work on this.

> 2) I did miss async versions of the algorithms, so it's possible to chain
> together multiple calls. Even though all the data sits on the compute
> device, the overhead of waiting for each operation to finish before queuing
> the next can make the compute gains completely irrelevant.

Can you let me know what chain of functions you're calling? Many
algorithms should already execute asynchronously and provide the
behavior you expect.

> 3) I think relevant calls should have a non-throwing form returning an error
> code, ala Boost.ASIO.

This could be implemented, but would be a large amount of work
(essentially doubling the size of the public API). Can you let me know
more about your use-case and why the current exception-based API is
not suitable?

> 4) Threading has been mentioned in this list. At the very least the
> documentation should be updated to clearly state the thread safety of each
> call/class, ala Boost.ASIO. Beyond that I would like to be able to share a
> context between several threads, each which could contain their own queue
> and device.

I'll add more documentation for this. You should be able to share a
context between multiple threads already, let me know if you run into
any issues with this.

> 5) If thread safety or similar important features requires a compiled part,
> I wouldn't mind Boost.Compute being non-header-only. Though if possible
> making it optional like other Boost libraries do would be great.

I have been considering this more, but it would be a large change (and
would be a non-backwards-compatible for existing users).

> 6) The tests, and preferably the performance tests as well, should have some
> way of testing all devices across all platforms. Currently they only test
> the default device on the default platform it seems. This is insufficient
> IMHO.

I have wanted to improve the testing infrastructure to run all tests
for all devices automatically, I just don't know the best way to use
Boost.Test to accomplish this. For now I manually run the tests for
each device (usually by setting BOOST_COMPUTE_DEFAULT_DEVICE and then
running ctest).

> 7) I'd like some way of defining user functions either as lambda
> expressions, raw OpenCL-C code via make_function_from_source or both, which
> can be used in lambda expressions for say transform(). Something like
>
>   function<int (int)> sqr = _1 * _1;
>
>   transform(inp.begin(), inp.end(), out.begin(), sqr(2 * _1 + 1)), queue);

This is definitely something I want to support, just haven't had the
time to implement it yet. There is an issue open already for it [1].

> Overall I think Boost.Compute can be useful right now for some use-cases,
> and it has good potential to be significantly more useful with a bit more
> work.

Thanks. I'll definitely continue working to fix these issues (as well
as others that came up during the review).

-kyle

[1] https://github.com/kylelutz/compute/issues/59