Boost logo

Boost :

From: Richard Hodges (hodges.r_at_[hidden])
Date: 2022-05-10 20:53:53


On Tue, 10 May 2022 at 22:33, Marcelo Zimbres Silva via Boost <
boost_at_[hidden]> wrote:

> Hi, here is my review.
>
> 1) ================================
>
> >
> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__resultset/async_read_all.html
> > The handler signature for this operation is
> > void(boost::mysql::error_code, std::vector<boost::mysql::row>).
> > etc.
>
> Having a std::vector in the signature of the completion handler is
> unusual in ASIO. In general, large objects should be passed as
> parameters to the initiating function.
>
> Additionally, this operation does not support custom allocators. That
> wouldn't be a big problem to me if I could reuse the same memory for
> each request but the interface above prevents me from doing so.
>
> It is also a pessimized storage as each row will allocate its own
> chunk of memory. A single block of memory would be better.
>
> 2) ================================
>
> >
> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__connection/async_query/overload1.html
> > The handler signature for this operation is
> > void(boost::mysql::error_code, boost::mysql::resultset<Stream>)
>
> Here the completion handler is returning an io object i.e.
> boost::mysql::resultset, which is also unusual as far as I can see.
> Wouldn't it make sense for users to specify a different executor for
> the resultset? This interface is preventing that.
>
> I prefer having the completion handler return config parameters with
> which I can instantiate a boost::mysql::resultset myself in my desired
> executor.
>

I seem to remember this being commented on during the development of the
library.
It is worth remembering that because this is MySQL, nothing can be done
with the connection object until the resultset has been fully read.
In effect, the resultset can be thought of as a modal extension to the
connection.
Causing it to run on a different executor would mean that calls to the
resultset would need to be marshalled to the connection's executor in any
case, and the opposite in reverse.
This may be of questionable value given the limitation of the underlying
protocol.
This arguably strengthens the case for the use of a connection pool, since
without one, as you say, frequent queries will have setup overhead.

The point about preferring not to return IO objects by value is a separate
stylistic issue, on which I offer no opinion.
However it is worth noting that Asio itself does have a form of the
ip::tcp::acceptor::async_accept initiation function whose completion
handler is passed a socket:
https://www.boost.org/doc/libs/1_79_0/doc/html/boost_asio/reference/basic_socket_acceptor/async_accept/overload3.html

>
> 3) ================================
>
> >
> https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee537/include/boost/mysql/value.hpp#L72
>
> I am not convinced the design of the value class is a good idea. The
> reason is that users know at compile time what types they are
> expecting from a given query. That means it would be possible for them
> to specify that before doing the query. So instead of doing this
>
>
> https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee537/example/query_async_coroutines.cpp#L107
>
> boost::mysql::row row;
> while (result.async_read_one(row, additional_info, yield[ec]))
>
> users could do
>
> boost::mysql::row<T1, T2, T3, T3> row;
> while (result.async_read_one(row, additional_info, yield[ec]))
>
> avoiding unnecessary further copies. In principle it would be even
> possible to parse directly in a custom data structure, for example,
> when storing json string in the database.
>
> 4) ================================
>
> > https://anarthal.github.io/mysql/mysql/examples.html
>
> IMO, the examples are too limited and show variation of the same
> functionality. For example
>
> - Text query, async with callbacks
> - Text query, async with futures
> - Text query, async with Boost.Coroutine coroutines
> - Text query, async with C++20 coroutines
> - Default completion tokens
>
> Here you are showing nice Asio features. I am not meaning you should
> remove them but if I am not mistaken there is no example that shows
>
> - How to keep long lasting connections open (specially for ssl).
> - Timers for resolve, connect, read and write.
> - Synchronized writes.
> - Healthy checks, failover etc.
>
> I estimate that to address these points users will be required to
> write further 500 - 1000 LOC on top of your library. Otherwise they
> will be stuck on underperforming short lived connections.
>
> It would be helpful for users to have a starting point to begin with.
>
> There some minor things I have also noticed:
>
> 5) ================================
>
> >
> https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee537/include/boost/mysql/impl/error.hpp#L35
>
> I expect this function to be O(1).
>
> 6) ================================
>
> >
> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_std_optional.html
> >
> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_optional.html
>
> Sounds also unusual to have two member functions for the different
> versions of optional. I suggest using the boost version until we
> transition to C++17.
>
> 7) ================================
>
> > https://anarthal.github.io/mysql/mysql/ref/boost__mysql__errc.html
>
> errc should be conditions and not error AFAICS.
>
> ============= Review End ================
>
> I vote for conditional acceptance. Points 1, 2, 3 and 4 should be
> addressed before acceptance. My acceptance criteria sums up to
>
> 1) Remove functionality that provides low performance. With
> coroutines it will be easy for users to loop on async_query to create
> a std::vector of rows.
>
> 2) No IO object should be returned as a parameter of the completion
> handler.
>
> 3) Review the value type. Can it be improved?
>
> 4) Improve examples.
>
> Regards,
> Marcelo
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk