Move message queue to Consumer #87

jakobod · 2020-10-01T14:24:45Z

This PR moves the message and event queues from its previous location in the endpoint_manager to the consumer.

This PR is based on #86 and should be merged after that PR has been merged.

jakobod · 2020-10-02T06:51:26Z

Sorry for the second force-push. I used an old master for the first rebase..
Ready for review now though!

libcaf_net/caf/net/consumer.hpp

Neverlord

I didn't look into the code in much detail yet.

May I suggest a change of perspective? The PR title reads like this is a simple refactoring step, while the proposed changes completely change the way BASP operates. Hence, this is in fact a major redesign. Shifting BASP to a message-oriented design opens up a lot of design space that seems mostly ignored. As a result, many pieces seem out of place to me and if there has been some thought put into it, there isn't any documentation or comment that communicates it.

On a side note, we moved away from the endpoint-manager design, because it forced BASP-specific notations into the lower API layers. Reusing code may be a good thing, but only after establishing a cohesive design.

It may also be a good idea to file non-architectural changes separately. For example, it seems like length_prefix_framing received a lot of changes (fixes?) and there's a new message_oriented_layer_ptr class. Those changes ought to be much less impactful and discussing/merging these changes may help keeping this PR focused.

Neverlord · 2020-10-04T12:58:24Z

libcaf_net/caf/net/actor_proxy_impl.hpp

@@ -37,7 +37,7 @@ class actor_proxy_impl : public actor_proxy {
  void kill_proxy(execution_unit* ctx, error rsn) override;

 private:
-  endpoint_manager_ptr dst_;
+  basp::application* app_;


This is a very serious shift in semantics. Previously, the proxy participated in shared ownership of the endpoint manager. Now, it only keeps a raw pointer to the BASP app directly. How is CAF making sure that the proxy never accesses a pointer past the lifetime of the pointed-to object now?

I couldn't find any comments going in to detail on this, and actor_proxy_impl::enqueue calls into the object without any safeguarding. Furthermore, what is the rationale behind keeping this proxy implementation in the public API when it's clearly bound to BASP?

How is CAF making sure that the proxy never accesses a pointer past the lifetime of the pointed-to object now

It doesn't. The proxy would have to hold a socket_manager_ptr to ensure the lifetime of the BASP app, which is definitely possible. I'll add that.

Neverlord · 2020-10-04T12:59:37Z

libcaf_net/caf/net/basp/application.hpp

-  using hub_type = detail::worker_hub<worker>;
+  using input_tag = tag::message_oriented;
+
+  using byte_span = span<const byte>;


There's caf/byte_span.hpp now that defined const_byte_span.

Neverlord · 2020-10-04T13:07:58Z

libcaf_net/caf/net/basp/application.hpp

-    parent.transport().configure_read(receive_policy::exactly(next_read_size));
-    return none;
+  template <class LowerLayerPtr>
+  bool prepare_send(LowerLayerPtr& down) {


This function opts to not call can_send_more() on the lower layer and instead drain the queues completely. Is there some benchmarking data that suggests simply writing to the buffer no matter what results in better performance overall? If so, the code should contain some comments that explain this brute-force approach to the reader so that this knowledge also guides future writers.

There is no benchmark that gives such distinct insight into the stack. At least not as of right now.
I used this approach, since it was used libcaf_io with the basp_broker.

Neverlord · 2020-10-04T13:09:14Z

libcaf_net/caf/net/basp/application.hpp

+  // -- handling of outgoing messages and events -------------------------------
+
+  template <class LowerLayerPtr>
+  error dequeue_events(LowerLayerPtr& down) {


The function is named "dequeue events", but instead of events, it returns an error. Since there's also no output parameter for events, this function name only helps confusing readers and requires to actual look at the implementation to figure out what the function does.

How about consume_events/consume_messages then? That would match the naming scheme used within actors and the new actor_shell.

Neverlord · 2020-10-04T13:11:41Z

libcaf_net/caf/net/basp/application.hpp

-  error handle_resolve_response(packet_writer& writer, header received_hdr,
-                                byte_span received);
+  template <class LowerLayerPtr>
+  error dequeue_messages(LowerLayerPtr& down) {


I have the same reservations regarding function name as I have for dequeue_events.

Neverlord · 2020-10-04T13:19:01Z

libcaf_net/caf/net/basp/constants.hpp

@@ -30,7 +30,7 @@ namespace caf::net::basp {
 constexpr uint64_t version = 1;

 /// Size of a BASP header in serialized form.
-constexpr size_t header_size = 13;
+constexpr size_t header_size = 9;


With this PR, you push for a complete paradigm shift by making BASP message-oriented. Why stick to a fixed header size in this case? BASP already had some message types that pushed fields that semantically belonged to the header into the payload. Now that we no longer need to support a fixed-size header to begin with, why stick to this poor design?

Neverlord · 2020-10-04T14:00:21Z

libcaf_net/caf/net/consumer_queue.hpp

@@ -34,7 +34,7 @@

 namespace caf::net {

-class CAF_NET_EXPORT endpoint_manager_queue {
+class CAF_NET_EXPORT consumer_queue {


The name suggests to me that this class is very generic queue type for (any number of?) producers to write to that belongs to a single consumer. But that just isn't the case. The implementation is in fact very specifically tailored to the needs of the BASP application. I think it shouldn't be in the public caf::net API to begin with and the name gives no guidance as to what role this class actually addresses.

Neverlord · 2020-10-04T14:12:08Z

libcaf_net/caf/net/stream_transport.hpp

@@ -155,6 +159,7 @@ class stream_transport {
      CAF_LOG_ERROR("send_buffer_size: " << socket_buf_size.error());
      return std::move(socket_buf_size.error());
    }
+    owner->register_reading();


Why should the transport unconditionally decide that the application has to read something right away?

The lifetime of socket_manager instances is bound to them being registered for reading or writing. As soon as a socket_manager is not registered for either of both, it is dereferenced by the multiplexer and hence, removed. Managers are only registered for writing, when they actually have to write data which leaves registering any manager for reading to keep them from being removed right away.

jakobod requested a review from Neverlord October 1, 2020 14:24

jakobod force-pushed the topic/message-queue branch from b27874f to 4264e10 Compare October 2, 2020 06:25

Neverlord and others added 22 commits October 2, 2020 08:29

Bump CAF tag

b60f85b

Add message_oriented_layer_ptr

b702230

WIP: Refactoring BASP

16b709f

WIP: message rework of BASP

16ba14f

Add convenience function for writing messages

ec4d770

Complete Interfaces of BASP and length_prefixing

6a5ab93

Fix length prefixing

8c1ec37

Move message queue to consumer

e090e0a

Fix prefix_framing test

e6245bf

Fix BASP tests

b6dd051

WIP: Fix full-setup test

47c1f07

Implement proper event handling in BASP

b319da5

Fix ping_pong test

9ee1ee8

Fix debug build and actor_shell test

5b638bc

Fix example/web-socket after rebase

7f85a0c

Check other TAG version

6cdded1

Add mutex to application

12437c5

Simplify BASP state tracking

b8adfe4

Wait for fulfilled handshake in BASP

c9c4161

Use correct config value in basp

4016a88

Cleanup

d58e004

Add missing functions after rebase

8c592d2

jakobod force-pushed the topic/message-queue branch from 4264e10 to 8c592d2 Compare October 2, 2020 06:50

jakobod added 4 commits October 2, 2020 10:35

Remove basp specifics from socket_manager

ab2eb4d

Remove endpoint_manager(impl)

417cea0

Reenable worker and message-queue tests

f68d37b

Remove unnecessary resolve functions

228826c

Neverlord reviewed Oct 2, 2020

View reviewed changes

libcaf_net/caf/net/consumer.hpp Outdated Show resolved Hide resolved

Remove consumer interface

784c96e

Neverlord requested changes Oct 4, 2020

View reviewed changes

Honor can_send_more result in application

a51fef6

Neverlord marked this pull request as draft October 5, 2020 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move message queue to Consumer #87

Move message queue to Consumer #87

jakobod commented Oct 1, 2020

jakobod commented Oct 2, 2020 •

edited

Loading

Neverlord left a comment

Neverlord Oct 4, 2020

jakobod Oct 4, 2020

Neverlord Oct 4, 2020

Neverlord Oct 4, 2020

jakobod Oct 4, 2020

Neverlord Oct 4, 2020

jakobod Oct 4, 2020

Neverlord Oct 4, 2020

Neverlord Oct 4, 2020

Neverlord Oct 4, 2020

Neverlord Oct 4, 2020

jakobod Oct 4, 2020

Move message queue to Consumer #87

Are you sure you want to change the base?

Move message queue to Consumer #87

Conversation

jakobod commented Oct 1, 2020

jakobod commented Oct 2, 2020 • edited Loading

Neverlord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobod commented Oct 2, 2020 •

edited

Loading