Mir

Merge lp:~vanvugt/mir/fix-1628794 into lp:mir

Proposed by Daniel van Vugt on 2016-11-14
Status: Merged
Approved by: Cemil Azizoglu on 2016-11-16
Approved revision: 3824
Merged at revision: 3824
Proposed branch: lp:~vanvugt/mir/fix-1628794
Merge into: lp:mir
Diff against target: 50 lines (+7/-12)
1 file modified
src/client/buffer_stream.cpp (+7/-12)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1628794
Reviewer Review Type Date Requested Status
Andreas Pokorny (community) Approve on 2016-11-16
Cemil Azizoglu (community) Approve on 2016-11-16
Alan Griffiths 2016-11-14 Approve on 2016-11-16
Mir CI Bot continuous-integration Approve on 2016-11-15
Review via email: mp+310760@code.launchpad.net

Commit message

Fix memory leak LP: #1628794, I think.

Also remove an unused (masked) member variable.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3823
https://mir-jenkins.ubuntu.com/job/mir-ci/2159/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2797/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2862
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2854
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2854
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2854
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2826/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2826/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2826/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2826/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2826/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2826/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2159/rebuild

review: Needs Fixing (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

Fix in the leak is one thing, but why are we doing a memory allocation at all?

It looks as though the unused member variable was intended for use in these call sites and using it would avoid allocating memory on each RPC.

review: Needs Information
Andreas Pokorny (andreas-pokorny) wrote :

This seems unnecessary complex. Do we still have to put mirprotobuf types on heap?

If not a stack variable would do the job.

If this is still the case I think you can get away with a make_unique<mp::Void>() and an empty callback .. like google::protobuf::NewCallback(google::protobuf::DoNothig) (bleh I wish this would be less verbose) so ignore_response can be removed too.

review: Needs Fixing
Andreas Pokorny (andreas-pokorny) wrote :

PS: the unique_ptr<mp::Void> member that contains the ignored non existing response \o/ could be the yet unused class member..

Daniel van Vugt (vanvugt) wrote :

All good questions but those are issues I have already considered...

* Why do a memory allocation? Because each call completes asynchronously in the background and must point to a valid Void object and not NULL, or it will crash.

* Stack variables: Not an option. The call completes in the background so the result lives longer than the stack memory that initiated it.

* google::protobuf::DoNothing: Also not an option. The Void* must point to a valid Void object which exists long enough for protobuf to populate it and make the callback. The callback must reference the Void object and be the last reference to it so it gets freed.

* Void is not really void. It contains useful error messages and codes, which we should pay more attention to.

lp:~vanvugt/mir/fix-1628794 updated on 2016-11-15
3824. By Daniel van Vugt on 2016-11-15

Merge latest trunk

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3824
https://mir-jenkins.ubuntu.com/job/mir-ci/2167/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2808
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2873
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2865
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2865
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2865
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2837/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2167/rebuild

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

> All good questions but those are issues I have already considered...
>
> * Why do a memory allocation? Because each call completes asynchronously in
> the background and must point to a valid Void object and not NULL, or it will
> crash.

That doesn't answer the question: why not use the Void data member whose lifetime surrounds any RPC calls?

I suspect the answer is:

> * Void is not really void. It contains useful error messages and codes, which
> we should pay more attention to.

If we were to use a single object, then using these error codes /could/ be racy.

Some years ago I went through the IPC stack tuning it for performance - a big part of that was enabling reuse of protobuf objects to avoid memory allocations. I no longer understand this code well enough to know whether there is a race here. Let's just land the fix and worry about performance if it comes around again.

review: Approve
Cemil Azizoglu (cemil-azizoglu) wrote :

Just got bit by this on my branch.

LGTM.

review: Approve
Andreas Pokorny (andreas-pokorny) wrote :

ah understood this thing lives inside the callback...

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/buffer_stream.cpp'
2--- src/client/buffer_stream.cpp 2016-11-09 02:30:14 +0000
3+++ src/client/buffer_stream.cpp 2016-11-15 02:24:53 +0000
4@@ -72,9 +72,8 @@
5 buf_params->set_pixel_format(format);
6 buf_params->set_buffer_usage(usage);
7
8- //note, NewCallback will trigger on exception, deleting this object there
9- auto protobuf_void = new mp::Void;
10- server.allocate_buffers(&request, protobuf_void,
11+ auto protobuf_void = std::make_shared<mp::Void>();
12+ server.allocate_buffers(&request, protobuf_void.get(),
13 google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
14 }
15
16@@ -84,9 +83,8 @@
17 request.mutable_id()->set_value(stream_id);
18 request.add_buffers()->set_buffer_id(buffer_id);
19
20- //note, NewCallback will trigger on exception, deleting this object there
21- auto protobuf_void = new mp::Void;
22- server.release_buffers(&request, protobuf_void,
23+ auto protobuf_void = std::make_shared<mp::Void>();
24+ server.release_buffers(&request, protobuf_void.get(),
25 google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
26 }
27
28@@ -96,20 +94,17 @@
29 request.mutable_id()->set_value(stream_id);
30 request.mutable_buffer()->set_buffer_id(buffer.rpc_id());
31
32- //note, NewCallback will trigger on exception, deleting this object there
33- auto protobuf_void = new mp::Void;
34- server.submit_buffer(&request, protobuf_void,
35+ auto protobuf_void = std::make_shared<mp::Void>();
36+ server.submit_buffer(&request, protobuf_void.get(),
37 google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
38 }
39
40- static void ignore_response(mp::Void* void_response)
41+ static void ignore_response(std::shared_ptr<mp::Void>)
42 {
43- delete void_response;
44 }
45
46 private:
47 mclr::DisplayServer& server;
48- mp::Void protobuf_void;
49 int stream_id;
50 };
51

Subscribers

People subscribed via source and target branches