Mir

Merge lp://qastaging/~albaguirre/mir/fix-1428402 into lp://qastaging/mir

Proposed by Alberto Aguirre
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 2373
Proposed branch: lp://qastaging/~albaguirre/mir/fix-1428402
Merge into: lp://qastaging/mir
Diff against target: 44 lines (+11/-5)
2 files modified
src/server/frontend/protobuf_responder.cpp (+9/-5)
src/server/frontend/protobuf_responder.h (+2/-0)
To merge this branch: bzr merge lp://qastaging/~albaguirre/mir/fix-1428402
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Alan Griffiths Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+251852@code.qastaging.launchpad.net

Commit message

Fix data race in ProtobufResponder::send_response

Description of the change

Fix data race in ProtobufResponder::send_response

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Looks like send_response_result doesn't need to be a class member at all (?)

If not, then convert it to a local variable and no locking is required. Lock-less it might perform better, despite having to construct a local each time.

review: Needs Information
Revision history for this message
Kevin DuBois (kdub) wrote :

> Looks like send_response_result doesn't need to be a class member at all (?)

+1, it looks like this would work, and the MessageSender/ResourceCache have locks around their internals.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Looks like send_response_result doesn't need to be a class member at all (?)
>
> If not, then convert it to a local variable and no locking is required. Lock-
> less it might perform better, despite having to construct a local each time.

IIRC this became a class member based on profiling a long time ago (it being "good practice to reuse protobuf objects"). I think in those days we only had one thread accessing it.

As contention free locks are fairly cheap I'm not convinced making it local would be a performance gain.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

*Needs Discussion*

I wonder whether the better fix is to ensure there is only a single IPC thread running and dispatch all activity to that thread.

review: Needs Information
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> *Needs Discussion*
>
> I wonder whether the better fix is to ensure there is only a single IPC thread
> running and dispatch all activity to that thread.

By default the IPC thread pool is set to 1 but it's configurable option (--ipc-thread-pool)

The other thread involved is the compositor thread, because of the completion given to surface.swap_buffers in SessionMediator::exchange_buffer (which we could make synchronous).

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

So then we have three options:

1. Make send_response_result a local
2. Protect send_response_result
3. Make SessionMediator::exchange_buffer synchronous (i.e. don't execute done->Run() in the surface.swap_buffers completion function, instead wait for the completion to occur) and remove the --ipc-thread-pool option

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

So option 3 is out of the question, since it would block the IPC thread.

Option 1 would potentially have performance impacts (protobuf objects could be heavy to construct).

So I rather keep the current optimization, protect it with a lock. When we re-profile IPC we can then revisit this decision.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

OK, good enough for a bugfix.

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

It's not a blocker if we don't know the answer...

The only issue to consider is that CPU hogs (excessive construction) are easy to find because any profiler can find them. Real-time wasters (excessive locking or sleeping) are harder; only real-time profilers like Google Profiler or Rational Quantify will notice those.

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

We are presently assuming no thread does:
  send_response_result.set_events(...
If one did then the threads could interfere with each other.

Otherwise OK.

review: Approve

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