Skip to content

/ servo

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

Invert control flow, fix resizing, and improve checkerboarding significantly #3761

Closed
wants to merge 1 commit into from
Closed

Invert control flow, fix resizing, and improve checkerboarding significantly #3761

wants to merge 1 commit into from

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Oct 21, 2014

r? @glennw @mbrubeck
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Oct 21, 2014

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

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.
Cargo.lock Outdated
@@ -33,7 +33,7 @@ dependencies = [
"egl 0.1.0 (git+https://github.com/servo/rust-egl#88f2a13812ddbce2bf2317221663a61c31b3e220)",

This comment has been minimized.

Sign in to view
Copy link
@glennw

glennw Oct 22, 2014

Member

The Cargo.lock in ports/android/glut_app should be updated too.

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Oct 23, 2014

Author Contributor

Done.
components/compositing/compositor.rs Outdated
let have_unused_buffers = unused_buffers.len() > 0;
self.recomposite = self.recomposite || have_unused_buffers;
if have_unused_buffers {
let no_unused_buffers = unused_buffers.len() == 0;

This comment has been minimized.

Sign in to view
Copy link
@glennw

glennw Oct 22, 2014

Member

Why no_unused_buffers here? have_unused_buffers seems clearer and doesn't require the !
components/compositing/refresh_driver.rs Outdated
/// The number of nanoseconds per millisecond.
static NS_PER_MS: u64 = 1_000_000;

/// FIXME(pcwalton): This is NTSC; do we have to do anything different elsewhere?

This comment has been minimized.

Sign in to view
Copy link
@glennw

glennw Oct 22, 2014

Member

I think this comment can be removed.
components/compositing/refresh_driver.rs Outdated
ScheduleRefreshMsg(time) => {
let target = time as i64 + (REFRESH_RATE as i64) - (COMPOSITING_TIME as i64);
let delta = target - time::precise_time_ns() as i64;
timer::sleep(Duration::nanoseconds(delta));

This comment has been minimized.

Sign in to view
Copy link
@glennw

glennw Oct 22, 2014

Member

On Windows in particular, the OS sleep is often +/- 10ms, sometimes more, which seems like it would cause issues implementing the refresh driver this way? Do you think it's feasible to drive it directly from vsync?

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Oct 22, 2014

Author Contributor

How would we drive it from vsync in OpenGL? The message is sent immediately after glfwSwapBuffers() returns.

This comment has been minimized.

Sign in to view
Copy link
@glennw

glennw Oct 22, 2014

Member

I'm probably missing something completely obvious, but I'm not sure why the compositor can't just run at 60 fps (locked to vsync), like a game style render loop - what is the reason for having the refresh driver being a separate task?
@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 23, 2014

I think what's causing problems here is that I'm trying to do too much in this "refresh driver". What I'm doing here is not a refresh driver at all—it's a scrolling timer. All I want to do is to allow scrolling events to bunch up for 12ms before we time out and composite anyway (and show a checkerboard) after a scroll event. This problem is specific to scrolling because "normal" page updates initiated by the page itself don't have this problem: they paint themselves instead of having the compositor request painting.

Building a "scrolling timer" is a much more self-contained task than making a "refresh driver".

The solution, I think, is to put scrolling only on this "scrolling timer". All other updates just go through regular glfwSwapBuffers(), locked to vsync, just like a normal OpenGL app.
@pcwalton pcwalton force-pushed the pcwalton:smooth-scrolling branch from d58440f to 715ad0d Oct 23, 2014
@pcwalton pcwalton changed the title Invert control flow, fix resizing, improve checkerboarding significantly, and add a refresh driver Invert control flow, fix resizing, and improve checkerboarding significantly Oct 23, 2014
@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 23, 2014

@glennw re-r? I converted the "refresh driver" into a system that simply delays composites for scroll messages to give tiles time to paint when you scroll to them. Compositing happens immediately for other kinds of messages (unless compositing later in the frame was inevitable due to a scroll event). This feels a bit simpler/less fragile/more conventional.
@pcwalton pcwalton force-pushed the pcwalton:smooth-scrolling branch 2 times, most recently from aa50241 to 9e99f7c Oct 23, 2014
bors-servo pushed a commit that referenced this pull request Oct 23, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=glennw
Loading status checks…
b0a5915 
r? @glennw @mbrubeck
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 24, 2014

Mac:

window.rs:369:9: 369:37 error: unresolved name `glfw::Glfw::post_empty_event`.
window.rs:369         glfw::Glfw::post_empty_event()
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

Linux:

make[1]: Leaving directory `/home/servo/buildbot/slave/linux/build/target/native/glfw-sys-362ff9a8d4dbbbbe'

--- stderr
/home/servo/.cargo/git/checkouts/glfw-4fd4342c6777c12d/cargo-3.0.4/src/x11_window.c: In function '_glfwPlatformPostEmptyEvent':
/home/servo/.cargo/git/checkouts/glfw-4fd4342c6777c12d/cargo-3.0.4/src/x11_window.c:1206:43: error: '_GLFWlibraryX11' has no member named '_NULL'
     event.xclient.message_type = _glfw.x11._NULL;
                                           ^
make[3]: *** [src/CMakeFiles/glfw.dir/x11_window.c.o] Error 1
make[2]: *** [src/CMakeFiles/glfw.dir/all] Error 2
make[1]: *** [all] Error 2
make: *** [all] Error 2
@pcwalton pcwalton force-pushed the pcwalton:smooth-scrolling branch from 9e99f7c to 9569179 Oct 31, 2014
bors-servo pushed a commit that referenced this pull request Oct 31, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=glennw
Loading status checks…
2595924 
r? @glennw @mbrubeck
@pcwalton pcwalton force-pushed the pcwalton:smooth-scrolling branch 2 times, most recently from cd21233 to c387cde Oct 31, 2014
bors-servo pushed a commit that referenced this pull request Nov 1, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=glennw
Loading status checks…
9f2d432 
r? @glennw @mbrubeck
@pcwalton pcwalton force-pushed the pcwalton:smooth-scrolling branch from c387cde to 875932e Nov 1, 2014
bors-servo pushed a commit that referenced this pull request Nov 1, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=glennw
Loading status checks…
39c823a 
r? @glennw @mbrubeck
@pcwalton pcwalton force-pushed the pcwalton:smooth-scrolling branch from 875932e to c9f2b82 Nov 1, 2014
bors-servo pushed a commit that referenced this pull request Nov 1, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=glennw
Loading status checks…
64fd7f1 
r? @glennw @mbrubeck
@pcwalton
Copy link
Contributor Author

pcwalton commented Nov 1, 2014

@bors: retry
bors-servo pushed a commit that referenced this pull request Nov 1, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=glennw
Loading status checks…
e31a0ef 
r? @glennw @mbrubeck
@jdm
Copy link
Member

jdm commented Nov 3, 2014

This timed out while running linux reftests on both runs. Does this pass locally for you?
@pcwalton
Invert control flow, fix resizing, and improve checkerboarding
Loading status checks…
10f7b49 
significantly by giving tiles some time to paint before we render
unrendered content.
@pcwalton pcwalton force-pushed the pcwalton:smooth-scrolling branch from c9f2b82 to 10f7b49 Nov 4, 2014
@larsbergstrom

This comment has been minimized.

Sign in to view
Copy link

larsbergstrom commented on 10f7b49 Nov 5, 2014

r+

This comment has been minimized.

Sign in to view
Copy link

jdm replied Nov 5, 2014

@bors: retry
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented on 10f7b49 Nov 5, 2014

saw approval from larsbergstrom
at pcwalton@10f7b49

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

merging pcwalton/servo/smooth-scrolling = 10f7b49 into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

pcwalton/servo/smooth-scrolling = 10f7b49 merged ok, testing candidate = 6d81e39

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

some tests failed:
failure: http://build.servo.org/builders/mac1/builds/70

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

saw approval from larsbergstrom
at pcwalton@10f7b49

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

merging pcwalton/servo/smooth-scrolling = 10f7b49 into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

pcwalton/servo/smooth-scrolling = 10f7b49 merged ok, testing candidate = ffae110

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

all tests pass:
success: http://build.servo.org/builders/linux/builds/527
success: http://build.servo.org/builders/mac1/builds/71
success: http://build.servo.org/builders/mac2/builds/70

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 5, 2014

fast-forwarding master to auto = ffae110
bors-servo pushed a commit that referenced this pull request Nov 5, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=larsbergstrom
Loading status checks…
6d81e39 
r? @glennw @mbrubeck
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Nov 5, 2014

Failure is:

Unexpected Results
==================

/XMLHttpRequest/xmlhttprequest-timeout-worker-overrides.html
------------------------------------------------------------
FAIL Timeout test: timeout enabled after initially disabled, original timeout at 0, reset at 400 to 1000
15:30.92 LOG: MainThread INFO Closing logging queue
bors-servo pushed a commit that referenced this pull request Nov 5, 2014
bors-servo
auto merge of #3761 : pcwalton/servo/smooth-scrolling, r=larsbergstrom
Loading status checks…
ffae110 
r? @glennw @mbrubeck
@bors-servo bors-servo closed this Nov 5, 2014
mbrubeck added a commit to mbrubeck/servo that referenced this pull request Nov 7, 2014
@mbrubeck
Build fixes for Android windowing code
b85adf8 
This begins porting the Android event loop to work with the inverted flow
control from servo#3761.  Unfortunately, GLUT does not give us enough control over
the event loop to really make this work, so this will build but it may not run
properly.  Our current plan is to get rid of GLUT and switch to Glutin in the
near future.

r? @pcwalton
@mbrubeck mbrubeck mentioned this pull request Nov 7, 2014
Merged
bors-servo pushed a commit that referenced this pull request Nov 8, 2014
bors-servo
auto merge of #3925 : mbrubeck/servo/glut-loop, r=pcwalton
Loading status checks…
6ec5b41 
This begins porting the Android event loop to work with the inverted flow control from #3761.  Unfortunately, GLUT does not give us enough control over the event loop to really make this work, so this will build but it may not run properly.  Our current plan is to get rid of GLUT and switch to Glutin in the near future.

r? @pcwalton
bors-servo pushed a commit that referenced this pull request Nov 8, 2014
bors-servo
auto merge of #3925 : mbrubeck/servo/glut-loop, r=pcwalton
Loading status checks…
25e9830 
This begins porting the Android event loop to work with the inverted flow control from #3761.  Unfortunately, GLUT does not give us enough control over the event loop to really make this work, so this will build but it may not run properly.  Our current plan is to get rid of GLUT and switch to Glutin in the near future.

r? @pcwalton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@pcwalton @hoppipolla-critic-bot @larsbergstrom @jdm @glennw @bors-servo
You can’t perform that action at this time.