Merge lp://staging/~compiz-team/compiz/compiz.fix_1006335 into lp://staging/compiz/0.9.8

Proposed by Sam Spilsbury
Status: Merged
Merged at revision: 3243
Proposed branch: lp://staging/~compiz-team/compiz/compiz.fix_1006335
Merge into: lp://staging/compiz/0.9.8
Diff against target: 303 lines (+98/-47)
4 files modified
src/privatescreen.h (+22/-2)
src/privatestackdebugger.h (+9/-4)
src/screen.cpp (+37/-30)
src/stackdebugger.cpp (+30/-11)
To merge this branch: bzr merge lp://staging/~compiz-team/compiz/compiz.fix_1006335
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Compiz Maintainers Pending
Review via email: mp+108706@code.staging.launchpad.net

This proposal supersedes a proposal from 2012-06-02.

Description of the change

Fixes needless list copying in event queues

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

There's a risk of crashing, by assuming there are still only XEventsQueued by the time we consume them all. The server might have more events ready by then. More than we sized the event vector for, so either:
        XNextEvent (dpy, &(*it));
or
        it++
will crash. Because it is now past the end of the vector.

But it's a simple fix. Just use reserve() instead of resize() and push_back instead of writing to *it.

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Also I'm still concerned by the use of the heap on every single event:

void
PrivateScreen::processEvents ()
{
    std::vector <XEvent> events;

The solution I had in mind for this bug didn't require that. Maybe I'll prototype it to see if it works.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

OK, my first comment is mistaken. But I'd like to see a solution that bypasses the heap for most events.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

That's a lot more complicated than the solution I had in mind, but if it works...

Also, I recommend not changing unrelated logic like the location of:
    windowManager.removeDestroyed ();
The change may be correct, but changing logic not related to the bug in question is best avoided.

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

On Fri, 1 Jun 2012, Daniel van Vugt wrote:

> Review: Needs Fixing
>
> That's a lot more complicated than the solution I had in mind, but if it works...
>
> Also, I recommend not changing unrelated logic like the location of:
> windowManager.removeDestroyed ();
> The change may be correct, but changing logic not related to the bug in question is best avoided.

Actually I changed it because we should be removing destroyed windows at
the end of every event processed. Without that change, we might never do
it.

Could you clarify what else needs fixing?

> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1006335/+merge/107953
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~compiz-team/compiz/compiz.fix_1006335 into lp:compiz.
>

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

windowManager.removeDestroyed ();

is what needs fixing. Your reasoning for moving it sounds correct but it's not related to bug 1006335 so please remove it from the proposal.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Sorry, I just noticed:
    std::deque
should be:
    std::list

Because std::deque introduces extra complexity to implement random access. It's actually implemented as a red-black tree like std::map or std::set. But since we're not using random access and only need head/tail insertion and removal, a simple doubly linked list (std::list) will be faster and smaller.

This proposal is still much larger than it should be. It's an itch I have to scratch but I'm trying not to push the point too much. :/

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Thanks, recompiling.

I don't think that the proposal is too large, however it does optimize bits that are only really used in debug mode, which pushed up the size a bit.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Done.

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

Looks OK and no obvious regressions.

I can't do a realistic comparative profile with callgrind right now because I've switched drivers and it would change my normal expected results. But this appears to solve the bug.

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