Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compositing: Move compositing off the main thread. #4271

Closed
wants to merge 2 commits into from

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Dec 7, 2014

Because the compositor no longer runs on the main thread, it can now use
plain old Rust channels for communication, and the main thread instead
receives the special "proxy" treatment. This change also makes the
compositor entirely optional; in headless mode, no compositor is started
at all.

This change will allow us to support multiple windows, since those must
have separate compositors. It should also improve our event dispatch
latency, since even less runs on the main thread, and we can handle
events while waiting for buffer swaps.

Additionally, this patch refactors many of the task startup functions,
which took too many arguments, to use initialization structures. This
makes the code easier to read.

Closes #4228.

r? @glennw

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Dec 7, 2014

Critic review: https://critic.hoppipolla.co.uk/r/3409

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 7, 2014

No OS specific stuff was needed because GLFW supplies the necessary functions. This was nice.

pcwalton added 2 commits Dec 6, 2014
Because the compositor no longer runs on the main thread, it can now use
plain old Rust channels for communication, and the main thread instead
receives the special "proxy" treatment. This change also makes the
compositor entirely optional; in headless mode, no compositor is started
at all.

This change will allow us to support multiple windows, since those must
have separate compositors. It should also improve our event dispatch
latency, since even less runs on the main thread, and we can handle
events while waiting for buffer swaps.

Additionally, this patch refactors many of the task startup functions,
which took too many arguments, to use initialization structures. This
makes the code easier to read.

Closes #4228.
@pcwalton pcwalton force-pushed the pcwalton:omtc branch from 85fde22 to 7c67fdb Dec 8, 2014
@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 8, 2014

@glennw Updated with @Ms2ger's comments addressed.

@glennw
Copy link
Member

glennw commented Dec 8, 2014

@pcwalton I checked out your branch, but the running the ref-tests hang on linux, and pressing escape or clicking close on about-mozilla.html gives the following:

task '' panicked at 'You should have disposed of the pixmap properly with destroy()! This pixmap will leak!', /home/gw/.cargo/git/checkouts/rust-layers-3d9d5f0b4e4d9002/master/src/platform/linux/surface.rs:206
task 'Constellation' panicked at 'sending on a closed channel', /home/jack/src/rust/src/libsync/comm/mod.rs:571

@glennw
Copy link
Member

glennw commented Dec 12, 2014

@pcwalton I had a brief look at this. The root of the problem appears to be that compositor.shutdown() is never called with this patch.

This means that forget_all_tiles() isn't called, resulting in the panic above. If I comment out the panic!() when dropping a leaked surface, then the ref-tests run and pass.

With that change, there is another panic that occurs when closing the window, but I suspect this will be solved when a fix is made to shutdown the compositor as mentioned above.

@jdm jdm added the S-needs-rebase label Dec 19, 2014
@pcwalton
Copy link
Contributor Author

pcwalton commented Jan 29, 2015

@glennw Glutin has landed while this patch was outstanding. Is there a way to push events to the event queue from another thread with it?

@glennw
Copy link
Member

glennw commented Jan 29, 2015

@pcwalton Do you just need to wake up the event queue from another thread? You can use the glutin WindowProxy interface for that. If you need something more than just a wakeup event, let me know what's required and I'll add it.

@metajack
Copy link
Contributor

metajack commented Mar 31, 2015

Assigning @glennw to this, but looks like @pcwalton needs to make the necessary change and rebase.

@glennw
Copy link
Member

glennw commented Apr 1, 2015

@pcwalton I think this just needs to be rebased and updated for glutin based on my last comment / question above.

@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

@pcwalton Sounds like this should be easy to land.

@nox
Copy link
Member

nox commented Sep 14, 2015

Is this PR even relevant with compositor being IpcSender<_> now?

@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 14, 2015

Yes. We will need this in order to support multiple windows, for example.

@nox
Copy link
Member

nox commented Sep 15, 2015

@pcwalton Would it help if we split this branch in more commits? I can try to separate this:

Additionally, this patch refactors many of the task startup functions, which took too many arguments, to use initialization structures. This makes the code easier to read.

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 15, 2015

I think that's worth doing independently of this PR.

@nox
Copy link
Member

nox commented Sep 15, 2015

@Ms2ger Yes that's what I was saying meaning.

bors-servo pushed a commit that referenced this pull request Sep 16, 2015
bors-servo
Introduce structs for compositing and script task constructors' arguments

Extracted from @pcwalton's #4271.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7645)
<!-- Reviewable:end -->
@nox
Copy link
Member

nox commented Sep 23, 2015

Things I remarked so far:

  • many messages sent to the compositor should be sent to the main thread directly (everything where the compositor used to just call something on self.window);
  • the compositor doesn't seem to need to send_opt() to the constellation on shutdown, because the compositor doesn't instantiate shutdown ever anymore;
  • I have absolutely no idea what to do about WindowMethods::prepare_for_compositing().
@glennw
Copy link
Member

glennw commented Nov 12, 2015

@pcwalton Should we close this and re-open it when you have time to look at this again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.