Mir

Merge lp://qastaging/~albaguirre/mir/server-improve-abi-compat-with-mirprotobuf into lp://qastaging/mir

  1. server-improve-abi-compat-with-mirprotobuf
  2. Merge into development-branch
Proposed by Alberto Aguirre
Status: Work in progress
Proposed branch: lp://qastaging/~albaguirre/mir/server-improve-abi-compat-with-mirprotobuf
Merge into: lp://qastaging/mir
Diff against target: 748 lines (+127/-122)
13 files modified
src/client/buffer_stream.cpp (+9/-9)
src/client/mir_connection.cpp (+12/-12)
src/client/mir_prompt_session.cpp (+7/-7)
src/client/mir_screencast.cpp (+5/-5)
src/client/mir_surface.cpp (+13/-13)
src/client/rpc/mir_protobuf_rpc_channel.cpp (+3/-3)
src/include/common/mir/make_protobuf_object.h (+2/-5)
src/protobuf/mir_protobuf.proto (+24/-23)
src/server/frontend/event_sender.cpp (+23/-22)
src/server/frontend/protobuf_message_processor.cpp (+11/-10)
src/server/frontend/protobuf_responder.cpp (+10/-6)
src/server/frontend/protobuf_responder.h (+1/-1)
src/server/frontend/socket_connection.cpp (+7/-6)
To merge this branch: bzr merge lp://qastaging/~albaguirre/mir/server-improve-abi-compat-with-mirprotobuf
Reviewer Review Type Date Requested Status
Robert Carr (community) Approve
Alexandros Frantzis (community) Approve
Chris Halse Rogers Approve
Daniel van Vugt Abstain
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+265453@code.qastaging.launchpad.net

Commit message

Improve ABI compatibility between mirserver and mirprotobuf.

server: Avoid allocating protobuf objects on the stack to avoid mismatched message definitions between server and future revision of the mirprotobuf library.

protobuf: Avoid class layout changes in protobuf generated code. The protobuf compiler uses order of appearance when generating classes and array offsets from a *.proto file. New fields should always go at the end of a message definition. New message definitions also need to be declared at the end of the file to avoid changes to some generated array index offsets.

Description of the change

Improve ABI compatibility between mirserver and mirprotobuf.

server: Avoid allocating protobuf objects on the stack to avoid mismatched message definitions between server and future revision of the mirprotobuf library.

protobuf: Avoid class layout changes in protobuf generated code. The protobuf compiler uses order of appearance when generating classes and array offsets from a *.proto file. New fields should always go at the end of a message definition. New message definitions also need to be declared at the end of the file to avoid changes to some generated array index offsets.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2766
http://jenkins.qa.ubuntu.com/job/mir-ci/4387/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3254
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/107
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/779
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3202
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/536
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/536/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3202
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3202/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6002
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22038

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

review: Approve (continuous-integration)
Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I had a different plan in mind that I hadn't shared yet:

1. Wait for 0.14.0 to be released in wily.
2. Then it should be safe for us to bump the ABI to libmirprotobuf.so.1, and keep bumping it with ABI breaks (message field changes) into the future.
3. Move protobuf message objects back onto the stack because it will then be safe to do so, and revert the performance regression of having them allocated on the heap for every message.

So that would be a "Disapprove" for this branch. But alternatively, you could consider it an "Abstain" as we may not need to revert most of the stack changes if they're not measurably slowing things down (needs some new profiling done).

review: Disapprove
Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Or we could keep using dynamic allocation without the performance penalty using:
https://developers.google.com/protocol-buffers/docs/reference/arenas

Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

It seems we can have the best of both worlds. The aforementioned options are all compatible possibilities.

review: Abstain
Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Looks sensible to me, to the extent that protobuf is sensible.

Probably a good idea to look at arenas as a follow up, but I'm happy to do don't-crash now and don't-crash-and-thrash-the-heap-less later. It looks like make_protobuf_object could be easily extended to take an arena, so I don't think this makes it any more difficult to do the performant thing later.

@Daniel: What change has made it safe to bump libmirprotobuf SONAME? Have we done something that will prevent multiple mirprotobufs from being loaded simultaneously?

review: Approve
Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I haven't verified it totally works yet, but the fix for bug 1391976 should stop the crash that prevented us from being able to bump the ABI to libmirprotobuf.so.1. So assuming that works, and assuming dynamic symbol resolution is correctly local to the bound SONAME, it might work. Or at least be a step forward. I forget all the details but a simple MP to test the water after 0.14.0 is released and installed on the CI machines will tell us more.

Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

I'm reasonably certain that #1391976 is not the bug preventing us from
loading libmirprotobuf.so.1 simultaneously with libmirprotobuf.so.0,
but it's certainly worth a bit of empiricism!

Reply
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

OK.

review: Approve
Reply
Revision history for this message
Robert Carr (robertcarr) wrote :

+1

review: Approve
Reply
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Andreas had issues when bumping the libmirprotobuf ABI but I'm not sure what they were.

From local experiments it does look doable. For additional safety, we can also version all its symbols.

Reply

Unmerged revisions

2766. By Alberto Aguirre

server:: Allocate mir::protobuf objects on the heap through their factory methods.

Avoid allocating protobuf objects on the stack to avoid mismatched message definitions between
server and mirprotobuf library. This improves ABI compatiblity with future revisions of mirprotobuf.

2765. By Alberto Aguirre

Avoid class layout changes in protobuf generated code to keep ABI compatibility in libmirprotobuf

The protobuf compiler uses order of apperance (not sequence numbers) when generating classes and table offsets.
New message definitions also need to be delcared at the end of the file to avoid changes to some generated array index offsets.

2764. By Alberto Aguirre

Move make_protobuf_object as it will be needed on server side as well

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches