Implemented #320. #337
Implemented #320. #337
Conversation
Added buffer class that supports `mqtt::string_view` compatible behavior and life keekping mechanism (optional). Callback functions for users hold receive buffer directly via `buffer`. Removed `*_ref` properties. Ref or not ref is hidden by `buffer`.
Status
Problem
masterimpl_320Time reportUsing masterimpl_320I want to improve compile time, but I don't come up with good ideas, so far. |
include/mqtt/message.hpp
Outdated
| template <typename Iterator> | ||
| basic_pubrel_message(Iterator b, Iterator e) | ||
| : base(b, e) | ||
| basic_pubrel_message(buffer buf) |
There was a problem hiding this comment.
Ah, yes MQTT v3.1.1's pubrel message is fixed length message. I will fix it.
There was a problem hiding this comment.
I changed as follows for consistency.
basic_pubrel_message(mqtt::string_view)
include/mqtt/message.hpp
Outdated
| auto qos = publish::get_qos(fixed_header_); | ||
| ++b; | ||
| buf = buf.substr(1); |
There was a problem hiding this comment.
You're not having the basic_publish_message object actually take ownership of the data that it's referring to.
So unless you plan to have basic_publish_message hold onto an instance of the passed in mqtt::buffer, this function should instead take an mqtt::string_view (by value), to avoid all of the lifetime management activity that's going on here.
include/mqtt/property.hpp
Outdated
| boost::container::static_vector<char, 2> len; | ||
| as::const_buffer str; | ||
| }; | ||
|
|
||
| struct len_str { |
There was a problem hiding this comment.
I don't understand what the purpose of this struct is.
boost::container::static_vector<char, 2> len; value can be calculated as needed, on the fly. No need to store it.
The logic from
if (!already_checked) {
auto r = utf8string::validate_contents(buf);
if (r != utf8string::validation::well_formed) throw utf8string_contents_error(r);
}
looks like it could be trivially merged into the UserProperty constructor, couldn't it?
There was a problem hiding this comment.
Even if we can't eliminate the "len_str" class entirely,
I think it should be moved into the private: section of the UserProperty class.
| inline | ||
| mqtt::optional<property_variant> parse_one(It& begin, It end) { | ||
| if (begin == end) return mqtt::nullopt; | ||
| std::pair<mqtt::optional<property_variant>, buffer> parse_one(buffer buf) { |
There was a problem hiding this comment.
I don't see why this function should return an std::pair.
The properties, if any are successfully parsed, will automatically preserve the lifetime of the mqtt::buffer object.
No need to explicitly return the buffer object.
Further, I don't think this function should have that responsibility. Callers of it should be aware that if no properties are returned, then there may be no references to buf. So they would need to ensure they manage the lifetime the way they want to.
Also, for functions like this one where the number of "assignments" of the parameter is unknown, I think it should be a const& parameter, instead of by-value.
There was a problem hiding this comment.
The reason I return buf as the second member of the pair is updating the buffer position to the next property starting point.
We can pass an iterator with buf. See the following code:
inline
std::pair<std::vector<property_variant>, buffer> parse(buffer buf) {
std::vector<property_variant> props;
auto it = buf.cbegin();
while (true) {
auto ret = parse_one(buf, it); // buf is passed by const reference,
// it is passed by non const reference
// it is updated to the next property start position
if (ret.first) {
props.push_back(std::move(ret.first.value()));
buf = std::move(ret.second);
}
else {
break;
}
}
return { props, std::move(buf) };
}Or we can also use #337 (comment) approach.
In addition parse doesn't need to return buffer. The caller knows whole properties size by Property Length field.
Any ideas?
There was a problem hiding this comment.
The property length field is always exactly the same, regardless of which type of property.
I would parse the property length outside of the parse_one function, and then create an MQTT::string_view with the whole data for the property. Then pass the MQTT::string_view by value, and but by const&.
If the parse_one function returns a property that needs to own a reference, then it will make a new MQTT::buffer using the lifetime from the const& buf, and the data from the MQTT::string_view.
There was a problem hiding this comment.
Property length field doesn't appear each properties.
I mean the data structure is as follows:
+-----------------+-----------+-----------+-----+-----------+
| Prioerty Length | Property1 | Property2 | ... | PropertyN |
+-----------------+-----------+-----------+-----+-----------+
We cannot know each property size and number of properties.
See https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901027
include/mqtt/property_parse.hpp
Outdated
| auto it = begin; | ||
| switch (static_cast<property::id>(*it++)) { | ||
| auto id = static_cast<property::id>(buf.front()); | ||
| buf = buf.substr(1); |
There was a problem hiding this comment.
Careful with calls to substr, this causes an atomic increment in the shared pointer, and then an atomic decrement.
There was a problem hiding this comment.
Yes, I should avoid unnecessary atomic increment and decrement via share_ptr_array's copy constructor.
I come up with https://wandbox.org/permlink/UramwEowhKy1oRFw. This could be another option.
I didn't check all locations I call substr() yet. If mqtt::string_view is obviously easy and clear, I use it.
There was a problem hiding this comment.
Using rvalue references is a good way to avoid the problem in the generic sense.
But I think that there is still room for improvement in the places where MQTT::buffer is used, as well.
There was a problem hiding this comment.
All buf = buf.substr(N) (buf : buffer) are replaced with buf = std::move(buf).substr() for the first step.
| @@ -714,7 +714,7 @@ class test_broker { | |||
| bool unsubscribe_handler( | |||
| Endpoint& ep, | |||
| typename Endpoint::packet_id_t packet_id, | |||
| std::vector<mqtt::string_view> topics, | |||
| std::vector<mqtt::buffer> topics, | |||
There was a problem hiding this comment.
There are several other places that need attention in this code as well.
E.g. see the "do_publish" function.
void do_publish(
std::shared_ptr<std::string> const& topic,
std::shared_ptr<std::string> const& contents,
std::uint8_t qos,
bool is_retain,
std::vector<mqtt::v5::property_variant> props) {There was a problem hiding this comment.
I believe that I fixed all part of test_broker using mqtt::buffer.
Please double check.
|
Thank you @jonesmz , your comments are very valuable. I will take a look them later. |
|
To test the compile time increase, you could try an experiment. If you made a new branch off of If they have not, then modify the various property types to use MQTT::buffer, and also MQTT::will, and see if compile times change. If those two parts of the code don't change the compile times, then we can assume its the finite state machine |
|
I'm on my phone so I can't say this as a comment on the line of code directly, sorry. I noticed in the various packet parser functions that you use "shared_from_this" for each lambda you create. I think it would be better, and use less atomic operations, if you instead had a "lifetime" parameter to the various state machine functions, which could be std::move'd as needed. Also, I noticed at the beginning of class endpoint that you can use "this_type" as part of the using statement for "using shared_from_this" to reduce copy-paste. |
|
Also, in the various packet parsing state machine functions: I noticed two things:
Instead, each function in the packet-parsing state machine functions can be a template function, and take the lambda function type directly, instead of needing to use std::function. I suspect that will help improve compile times. |
Replaced template property and will constructors with `mqtt::buffer` ones. Added `""_mb` user defined literals. Moved `len_str` to user_property's private member. Added rvalue version of `buffer::substr()`. Added `buffer::view()` to get `mqtt::string_view`.
settings
logresults
The All branches are on the github. You can double check them on your environment. |
|
The class template NOTE: The machine spec is different from #337 (comment).
I guess that nested classes in In addition, I use |
I did it but compile times doesn't improved.
I added void process_payload(async_handler_t func) {
#if 0
auto control_packet_type = get_control_packet_type(fixed_header_);
switch (control_packet_type) {
case control_packet_type::connect:
...
default:
break;
}
#endif
}Then compile times become 15.9 sec from 96.6 sec. So I guess that packet processing functions are dominant factor. |
void process_properties(
async_handler_t func,
buffer buf,
std::function<void(std::vector<v5::property_variant>, buffer, async_handler_t)> handler) {
#if 0
....
#endif
}When I add |
|
On the branch impl_320_struct_outside:
|
|
Let's focus on The target function is
Hmm, it seems that |
include/mqtt/endpoint.hpp
Outdated
| @@ -69,6 +71,7 @@ template <typename Socket, typename Mutex = std::mutex, template<typename...> cl | |||
| class endpoint : public std::enable_shared_from_this<endpoint<Socket, Mutex, LockGuard, PacketIdBytes>> { | |||
| using this_type = endpoint<Socket, Mutex, LockGuard, PacketIdBytes>; | |||
| public: | |||
| using std::enable_shared_from_this<endpoint<Socket, Mutex, LockGuard, PacketIdBytes>>::shared_from_this; | |||
There was a problem hiding this comment.
I ran into a problem earlier today.
The way enable_shared_from_this is used in the mqtt_cpp codebase makes it so that if someone inherits from mqtt::endpoint, then
Derived * p;
p->shared_from_this();
Returns an instance of std::shared_ptr<mqtt::endpoint> and not std::shared_ptr<Derived>
One way to solve that is to use the "Curiously Recurring Template Pattern", and have the endpoint<> class take as a template parameter the type of the most-derived type of the inheritance graph, and then pass that type to std::enable_shared_from_this<>.
There was a problem hiding this comment.
Hmm... CRTP is work well with Derived. But I couldn't a good way to support endpoint as leaf.
https://wandbox.org/permlink/2jSaOy3xkNgO9sAu
Any ideas?
There was a problem hiding this comment.
I introduced base_selector meta function. See https://wandbox.org/permlink/Mm74LrjbYOMq4AZo
I think that it works well.
There was a problem hiding this comment.
I think that the following approach is better:
https://wandbox.org/permlink/7r1s3XgsIxNpxtNB
No meta function required.
There was a problem hiding this comment.
BTW, the issue is not related to the PR. So it should be a separate issue.
include/mqtt/endpoint.hpp
Outdated
| auto sp = std::make_shared<std::string>(b, e); | ||
| restore_serialized_message(basic_publish_message<PacketIdBytes>(sp->begin(), sp->end()), sp); | ||
| auto size = static_cast<std::size_t>(std::distance(b, e)); | ||
| shared_ptr_array spa { new char[size] }; |
There was a problem hiding this comment.
In C++20, you can do instead:
std::make_shared<char[]>(size);
Which avoids a secondary allocation behind the scenes.
For now, using boost::make_shared<char[]>(size) will avoid that problem.
Perhaps introduce a new CMake option?
MQTT_USE_BOOST_SHARED_PTR
If it's enabled, then mqtt_cpp uses boost::shared_ptr, and can do this with one allocation instead of two.
If it's disabled, then do an #if cpp20 check, and if compiling with cpp 20, use std::make_shared<char[]>(size);
And lastly, use the current code if neither of the above are true.
There was a problem hiding this comment.
I think that the following code is good enough:
#if !defined(MQTT_SHARED_PTR_ARRAY_HPP)
#define MQTT_SHARED_PTR_ARRAY_HPP
#ifdef MQTT_STD_SHARED_PTR_ARRAY
#include <memory>
namespace mqtt {
using shared_ptr_array = std::shared_ptr<char []>;
using shared_ptr_const_array = std::shared_ptr<char const []>;
inline shared_ptr_array make_shared_ptr_array(std::size_t size) {
#if __cplusplus > 201703L // C++20 date is not determined yet
return std::make_shared<char[]>(size);
#else // __cplusplus > 201703L
return std::shared_ptr<char[]>(new char[size]);
#endif // __cplusplus > 201703L
}
} // namespace mqtt
#else // MQTT_STD_SHARED_PTR_ARRAY
#include <boost/shared_ptr.hpp>
#include <boost/smart_ptr/make_shared.hpp>
namespace mqtt {
using shared_ptr_array = boost::shared_ptr<char []>;
using shared_ptr_const_array = boost::shared_ptr<char const []>;
inline shared_ptr_array make_shared_ptr_array(std::size_t size) {
return boost::make_shared<char[]>(size);
}
} // namespace mqtt
#endif // MQTT_STD_SHARED_PTR_ARRAY
#endif // MQTT_SHARED_PTR_ARRAY_HPPThere was a problem hiding this comment.
Implemented. Use boost::shared_ptr and boost::make_shared by default.
`mqtt::socket` is type erased socket. It covers tcp, tls, ws, and wss. Compile times become faster. boost type_erasure is based on vtable emulation. So member functions of the `mqtt::socket` are called indirectly. The cost of the indirect call can be ignored in `mqtt_cpp` usecase. I've checked calling cost between the following three approach. 1. Traditional object oriented style virtual function call. 2. variant based visit call. 3. boost type_erasure. There aren't any siginificant difference. The approach 1 requires inheritance and (I guess) doesn't work well with template. The approach 2 needs to all actual types in the variant definition. So I choose approach 3.
Used boost asio buffer() overload for will.
| @@ -1,6 +1,6 @@ | |||
| sudo: false | |||
| language: cpp | |||
| dist: trusty | |||
There was a problem hiding this comment.
Why the change?
Ddid we change the minimum C++ standard version or something?
There was a problem hiding this comment.
FYI, I'm using mqtt_cpp on OpenWRT, which only has GCC 7.3, so I wouldn't be able to use the latest release if we need GCC 9 now.
There was a problem hiding this comment.
g++ outputs compile error due to g++'s bug. I need to avoid it.
I don't know how to install g++9 on trusty so I updated it.
There was a problem hiding this comment.
What was the compiler error? That's really weird, I did see any code that looked problematic?
There was a problem hiding this comment.
The compile error is happened on the following code:
[
self = shared_from_this()
]https://travis-ci.org/redboltz/mqtt_cpp/jobs/572260184#L1050
/home/travis/build/redboltz/mqtt_cpp/include/mqtt/endpoint.hpp:9949:52: error: cannot call member function ‘std::shared_ptr<_Tp> std::enable_shared_from_this<_Tp>::shared_from_this() [with _Tp = mqtt::endpoint<std::mutex, std::lock_guard, 2>]’ without object
self = shared_from_this(),
~~~~~~~~~~~~~~~~^~
I can avoid it adding redundant this-> as follows:
[
self = this->shared_from_this()
]However, if I do that, then MSVC reports the following compile error:
https://redboltz.visualstudio.com/redboltz/_build/results?buildId=469
C:\Program Files\Boost\1.69.0\include\boost-1_69\boost/pending/integer_log2.hpp(7): note: This header is deprecated. Use <boost/integer/integer_log2.hpp> instead.
Please define _WIN32_WINNT or _WIN32_WINDOWS appropriately. For example:
- add -D_WIN32_WINNT=0x0501 to the compiler command line; or
- add _WIN32_WINNT=0x0501 to your project's Preprocessor Definitions.
Assuming _WIN32_WINNT=0x0501 (i.e. Windows XP target).
D:\a\1\s\include\mqtt/endpoint.hpp(9982): error C2039: 'shared_from_this': is not a member of 'mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::process_connect_impl::<lambda_f447512e4e7450f0685e2f392f3b8c35>' [D:\a\1\s\build\test\as_buffer_async_pubsub_1.vcxproj]
with
[
Socket=mqtt::tcp_endpoint<boost::asio::ssl::stream<boost::asio::ip::tcp::socket>,boost::asio::io_context::strand>
]
D:\a\1\s\include\mqtt/endpoint.hpp(9983): note: see declaration of 'mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::process_connect_impl::<lambda_f447512e4e7450f0685e2f392f3b8c35>'
with
[
Socket=mqtt::tcp_endpoint<boost::asio::ssl::stream<boost::asio::ip::tcp::socket>,boost::asio::io_context::strand>
]
D:\a\1\s\include\mqtt/endpoint.hpp(9828): note: while compiling class template member function 'void mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::process_connect_impl(mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::async_handler_t,mqtt::buffer,mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::connect_phase,mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::connect_info &&)'
with
[
Socket=mqtt::tcp_endpoint<boost::asio::ssl::stream<boost::asio::ip::tcp::socket>,boost::asio::io_context::strand>
]
D:\a\1\s\include\mqtt/endpoint.hpp(9813): note: see reference to function template instantiation 'void mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::process_connect_impl(mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::async_handler_t,mqtt::buffer,mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::connect_phase,mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>::connect_info &&)' being compiled
with
[
Socket=mqtt::tcp_endpoint<boost::asio::ssl::stream<boost::asio::ip::tcp::socket>,boost::asio::io_context::strand>
]
D:\a\1\s\include\mqtt/client.hpp(40): note: see reference to class template instantiation 'mqtt::endpoint<Socket,std::mutex,std::lock_guard,2>' being compiled
with
[
Socket=mqtt::tcp_endpoint<boost::asio::ssl::stream<boost::asio::ip::tcp::socket>,boost::asio::io_context::strand>
]
It seems that MSVC misunderstood that this is internally created lambda class object.
To solve both incomplete compiler problems, I remove the redundant this-> and update g++.
There was a problem hiding this comment.
I normally want to use the latest compiler version, but I'm currently limited to GCC 7.3.0 (I am, of course, always working on ways to upgrade to newer compilers..., but currently not able to), so I hope we can find a different work around.
Why not do:
auto self = shared_from_this();
before creating the lambda?
I also pointed out that there's a lot of places where we're calling shared_from_this() repeatedly, and could instead pass the shared_ptr into the function that then creates the lambda, avoiding the need for repeated calls to shared_from_this().
There was a problem hiding this comment.
Moved shared_from_this() call to outside of lambda expressions.
In addition, minimized shared_from_this() call.
There was a problem hiding this comment.
I downgrade g++ version to 7.4. Not only to test the workaround but also codecov support. I couldn't install gcov-9.
|
I've checked compile times using So I removed Then compile times are decreased.
The indirect function call speed between three approaches are not so different. |
`mqtt::buffer` can be convert to `mqtt::string_view` autonatically.
| @@ -7758,7 +7758,10 @@ class endpoint : public std::enable_shared_from_this<endpoint<Socket, Mutex, Loc | |||
| if (connected_) { | |||
| connected_ = false; | |||
| mqtt_connected_ = false; | |||
| shutdown_from_server(*socket_); | |||
| { | |||
| boost::system::error_code ec; | |||
There was a problem hiding this comment.
Is there any meaningful check that can be made on the error_code?
I can't think of any. Some kind of log message perhaps?
There was a problem hiding this comment.
I want to avoid throwing exception. I think that it should be logged as warning level in the future.
include/mqtt/optional.hpp
Outdated
| @@ -31,6 +31,11 @@ using boost::optional; | |||
| using nullopt_t = boost::none_t; | |||
| static const auto nullopt = boost::none; | |||
|
|
|||
| template <typename T, typename U> | |||
There was a problem hiding this comment.
I forgot to erase it. At first, I thought that boost::optional requires in-place factory but I noticed that boost::optional also have emplace().
I will remove it.
| > | ||
| > | ||
| binary_property(property::id id, Buffer&& buf) | ||
| binary_property(property::id id, buffer buf) |
There was a problem hiding this comment.
Did this change help at all with the compile times?
There was a problem hiding this comment.
No. compile times is not affected by this change. This change is to avoid implicit conversion. Users need to pass mqtt::buffer or "abc"_mb explicitly.
See #337 (comment)
Am I understand correctly??
There was a problem hiding this comment.
I try to avoid using universal / forwarding references when the thing being "forwarded" will be stored into a member variable.
I like pushing the responsibility of providing a fully constructed object onto the caller of the function. It's much less typing, less template instantiation, and causes much less surprises than using forwarding references.
There was a problem hiding this comment.
Ok. I understand that you think this change is valuable even if compile times is not changed.
There was a problem hiding this comment.
Right.
I expected a small improvement to compile times, but mostly I thought it was an improvement to the API regardless of whether compile times improved.
This video explains a bit: https://www.youtube.com/watch?v=PNRju6_yn3o
Note that using the universal / forwarding references is technically better in terms of (completely unoptimized) runtime performance because it has slightly less calls to the move constructor, but that's not relevant for header-only code. The compiler will almost certainly inline the call to the function, and therefore we should already have very efficient outcomes.
include/mqtt/property.hpp
Outdated
| struct len_str { | ||
| explicit len_str(buffer b, bool already_checked = false) | ||
| : buf(std::move(b)), | ||
| len{ num_to_2bytes(static_cast<std::uint16_t>(buf.size())) } |
There was a problem hiding this comment.
boost::numeric_cast?
That way, if the current value of the buffer.size() is larger than max uint16_t, an exception is thrown.
| namespace mpl = boost::mpl; | ||
|
|
||
| template <typename Concept> | ||
| class shared_any : public boost::type_erasure::any<Concept, boost::type_erasure::_self&> { |
There was a problem hiding this comment.
I'm not sure I understand the goal here.
Can't std::any / boost::any hold a shared_ptr ?
There was a problem hiding this comment.
This is very difficult to explain. The goal is erasing type.
boost::type_erasure::any can call member functions without cast. See type_erased_socket.hpp.
It is one of runtime type erasure.
There was a problem hiding this comment.
https://www.boost.org/doc/libs/1_70_0/doc/html/boost_typeerasure.html is document.
Boost type erasure doesn't support shared_ptr directly. I need to adapt the pointee object that is held by shared_ptr to the concept.
The 2nd argument of boost::type_erasure_any is to support reference semantics.
Moved shared_from_thig() call to outside of lambda expressions for workaroung old g++ bug.
|
Here is current result. Good compile times.
|
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
+ Coverage 85.53% 86.09% +0.56%
==========================================
Files 34 37 +3
Lines 4889 6237 +1348
==========================================
+ Hits 4182 5370 +1188
- Misses 707 867 +160 |
Replaced static_cast with boost::numeric_cast if overflow could happen. Removed redundant static_cast.
If use boost (default), then use `boost::make_shared<char[]>(size)`.
If user defines MQTT_STD_SHARED_PTR_ARRAY,
if __cplusplus is greater than 201703L (means C++20 or later),
then `std::make_shared<char[]>(size)`,
otherwise `std::shared_ptr<char[]>(new size)`
is called.
|
I believe that the PR is ready to merge. |
|
Would you consider closing this PR and making a new one? Then I could review everything thoroughly without being confused by old comments. |
|
Yes, I just created #339. |
Added buffer class that supports
mqtt::string_viewcompatiblebehavior and life keekping mechanism (optional).
Callback functions for users hold receive buffer directly via
buffer.Removed
*_refproperties. Ref or not ref is hidden bybuffer.