Merge lp:~vanvugt/mir/method-index into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Avoid passing method names as strings over the protocol. You can save time,
memory and bandwidth by using the method index (integer) instead.

Sad to say this only makes a very tiny performance improvement. But it was worth a try. dispatch() dropped from 15% (of Mir) to 14%. Such a small improvement is only measurable with a profiler.

So take it or leave it...

[Daniel van Vugt (vanvugt)]

The performance improvement might be more significant under flooding conditions, such as when vsync is disabled (bug 1130553).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:609
http://jenkins.qa.ubuntu.com/job/mir-ci/446/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/465
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/348
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/451
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/451/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/446/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I'm surprised that this change has any measurable effect as ProtobufMessageProcessor::dispatch() was down amongst the noise in my recent profiling runs.

Using the index is likely a little more efficient but has a risk of breaking with protocol changes. (I'm pretty sure that our messages fit in a block regardless of using an index or a name.)

If we go this way, I think we should be extracting and checking the index -> name information once on startup (not once for every client message) as this probably loses a lot of any gain.

review: Abstain

[Daniel van Vugt (vanvugt)]

Yeah I had another prototype that actually enumerates the method indices dynamically and matches them to names. But then realized it was pointless. The index values don't change unless the protocol does.

Also, any approach to passing the method as an integer method index (from protobuf) creates the risk of protocol breakage. You can eliminate the risk by saying the integer is instead a forever-unique method identifier (defined and allocated separately to the protobuf method index).

I wasn't aware that protobuf used "blocks" that might be so large that the strings fit into them anyway. If that's true then it explains the lack of performance benefit. Though switching transport mechanisms to something other than protobuf could result in our use of strings becoming more significant a problem.

I'm going to reject this. The gain is not significant enough, for now.

review: Disapprove

[Robert Ancell (robert-ancell)]

I had a play with using an enum in lp:~robert-ancell/mir/method-enum (I think it has issues because we don't expose the protobuf types into the rest of the code). Using a barely defined index is worse than sending a string. An enum would be nicer and more consistent with binary protocols, but it might take some more work to the RPC code to make this work well.

Unmerged revisions

609. By Daniel van Vugt on 2013-04-19

Avoid passing method names as strings in every single invocation. You can
use their unique integer indices instead.

608. By Robert Carr on 2013-04-19

Input configuration must be held by shared_ptr. Fixes: https://bugs.launchpad.net/bugs/1170524.

Approved by PS Jenkins bot, Kevin DuBois.

607. By Alan Griffiths on 2013-04-18

config: documentation of the DefaultServerConfiguration member functions.

Approved by Alexandros Frantzis, PS Jenkins bot.

606. By Robert Ancell on 2013-04-18

Use mir::shell:Session instead of mir::frontend::Session in mir::shell::SessionContainer and mir::shell::FocusSequence - this stops a user of these classes having to cast to mir::shell::Session objects.

Approved by PS Jenkins bot.

605. By Robert Ancell on 2013-04-18

Remove unused include

604. By Robert Ancell on 2013-04-18

Make mir::shell::SessionContainer an abstract class with a default implementation in DefaultSessionManager.

Approved by PS Jenkins bot, Kevin DuBois.

603. By Daniel van Vugt on 2013-04-18

Remember to generate debug info (LP: #1170202). Fixes: https://bugs.launchpad.net/bugs/1170202.

Approved by Alexandros Frantzis, Alan Griffiths, PS Jenkins bot.

602. By Alexandros Frantzis on 2013-04-18

Rename Shell::shutdown() and friends to ::force_requests_to_complete() and improve shutdown testing.

Approved by PS Jenkins bot, Kevin DuBois, Alan Griffiths.

601. By Daniel van Vugt on 2013-04-18

Make client API function names more consistent so that they follow the
convention: namespace_class_method(Class *x, ...).

Approved by Kevin DuBois, Alexandros Frantzis, PS Jenkins bot.

600. By Daniel van Vugt on 2013-04-18

Don't waste time clearing a depth buffer we will never use. It just slows us
down.

Certainly many clients will use depth buffers, but the compositor should
never need one.

Approved by Kevin DuBois, Alan Griffiths, PS Jenkins bot.

=== modified file 'src/client/mir_basic_rpc_channel.cpp'
--- src/client/mir_basic_rpc_channel.cpp	2013-04-11 22:47:09 +0000
+++ src/client/mir_basic_rpc_channel.cpp	2013-04-19 09:49:28 +0000
@@ -92,7 +92,7 @@
     mir::protobuf::wire::Invocation invoke;
 
     invoke.set_id(next_id());
-    invoke.set_method_name(method->name());
+    invoke.set_method_index(method->index());
     invoke.set_parameters(buffer.str());
 
     return invoke;

=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2013-04-16 09:08:29 +0000
+++ src/server/frontend/protobuf_message_processor.cpp	2013-04-19 09:49:28 +0000
@@ -19,10 +19,11 @@
 #include "protobuf_message_processor.h"
 #include "mir/frontend/message_processor_report.h"
 #include "mir/frontend/resource_cache.h"
-
+#include <google/protobuf/descriptor.h>
 #include <boost/exception/diagnostic_information.hpp>
 
 #include <sstream>
+#include <cassert>
 
 namespace mfd = mir::frontend::detail;
 
@@ -145,55 +146,64 @@
 
 bool mfd::ProtobufMessageProcessor::dispatch(mir::protobuf::wire::Invocation const& invocation)
 {
-    report->received_invocation(display_server.get(), invocation.id(), invocation.method_name());
+    using namespace ::google::protobuf;
+    const ServiceDescriptor *service = display_server->GetDescriptor();
+    int index = invocation.method_index();
+    const MethodDescriptor *method = service->method(index);
+    const std::string name = method ? method->name() : to_string(index);
+
+    report->received_invocation(display_server.get(), invocation.id(), name);
 
     bool result = true;
 
     try
     {
-        // TODO comparing strings in an if-else chain isn't efficient.
-        // It is probably possible to generate a Trie at compile time.
-        if ("connect" == invocation.method_name())
-        {
+        switch (index)
+        {  // One feature Protobuf is missing is enums for these. So use ints..
+        case 0:
+            assert(name == "connect");
             invoke(&protobuf::DisplayServer::connect, invocation);
-        }
-        else if ("create_surface" == invocation.method_name())
-        {
+            break;
+        case 1:
+            assert(name == "disconnect");
+            invoke(&protobuf::DisplayServer::disconnect, invocation);
+            result = false;
+            break;
+        case 2:
+            assert(name == "create_surface");
             invoke(&protobuf::DisplayServer::create_surface, invocation);
-        }
-        else if ("next_buffer" == invocation.method_name())
-        {
+            break;
+        case 3:
+            assert(name == "next_buffer");
             invoke(&protobuf::DisplayServer::next_buffer, invocation);
-        }
-        else if ("release_surface" == invocation.method_name())
-        {
+            break;
+        case 4:
+            assert(name == "release_surface");
             invoke(&protobuf::DisplayServer::release_surface, invocation);
-        }
-        else if ("test_file_descriptors" == invocation.method_name())
-        {
-            invoke(&protobuf::DisplayServer::test_file_descriptors, invocation);
-        }
-        else if ("drm_auth_magic" == invocation.method_name())
-        {
+            break;
+        case 5:
+            assert(name == "select_focus_by_lightdm_id");
+            invoke(&protobuf::DisplayServer::select_focus_by_lightdm_id,
+                   invocation);
+            break;
+        case 6:
+            assert(name == "drm_auth_magic");
             invoke(&protobuf::DisplayServer::drm_auth_magic, invocation);
-        }
-        else if ("select_focus_by_lightdm_id" == invocation.method_name())
-        {
-            invoke(&protobuf::DisplayServer::select_focus_by_lightdm_id, invocation);
-        }
-        else if ("configure_surface" == invocation.method_name())
-        {
+            break;
+        case 7:
+            assert(name == "test_file_descriptors");
+            invoke(&protobuf::DisplayServer::test_file_descriptors,
+                   invocation);
+            break;
+        case 8:
+            assert(name == "configure_surface");
             invoke(&protobuf::DisplayServer::configure_surface, invocation);
-        }
-        else if ("disconnect" == invocation.method_name())
-        {
-            invoke(&protobuf::DisplayServer::disconnect, invocation);
-            result = false;
-        }
-        else
-        {
-            report->unknown_method(display_server.get(), invocation.id(), invocation.method_name());
-            result = false;
+            break;
+        default:
+            report->unknown_method(display_server.get(), invocation.id(),
+                name);
+            result = false;
+            break;
         }
     }
     catch (std::exception const& error)

=== modified file 'src/shared/protobuf/mir_protobuf_wire.proto'
--- src/shared/protobuf/mir_protobuf_wire.proto	2012-09-14 12:03:52 +0000
+++ src/shared/protobuf/mir_protobuf_wire.proto	2013-04-19 09:49:28 +0000
@@ -2,7 +2,7 @@
 
 message Invocation {
   required uint32 id = 1;
-  required string method_name = 2;
+  required int32  method_index = 2;
   required bytes  parameters = 3;
 }