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

Document modification should trigger immediate reflow #417

Closed
jdm opened this issue May 7, 2013 · 7 comments
Closed

Document modification should trigger immediate reflow #417

jdm opened this issue May 7, 2013 · 7 comments
Labels

Comments

@jdm
Copy link
Member

@jdm jdm commented May 7, 2013

Currently it sends a message to the content task to trigger a reflow, which means that content JS can cause an infinite number of these messages to pile up before they get serviced. We need to skip the message and go for the jugular instead.

@sethfowler
Copy link
Contributor

@sethfowler sethfowler commented May 7, 2013

That's unbounded queues for ya. But I'm not sure I agree that immediate reflow is the solution. Since reflow is expensive, you ideally want to coalesce as many updates as you can into each reflow operation. An alternative solution would be message coalescing (which I think we'll find very useful generally). At the most basic level we could implement it by setting flags, if we don't want to do something more sophisticated quite yet.

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented May 7, 2013

Are the messages carrying any information other than "hey, do a reflow"?

@jdm
Copy link
Member Author

@jdm jdm commented May 7, 2013

https://github.com/mozilla/servo/blob/master/src/servo/dom/document.rs#L66 and https://github.com/mozilla/servo/blob/master/src/servo/content/content_task.rs#L378 suggest no. Obviously content_changed will become smarter over time, however it's clear that the indirection of sending a message here is wrong, since the content task doing the sending is also the one one receiving.

@sethfowler
Copy link
Contributor

@sethfowler sethfowler commented May 9, 2013

Seems to me that we can't just call relayout directly from content_changed for two reasons - update coalescing, as mentioned above, and the fact that relayout has to block on the layout task, which seems unjustified to me. Triggering immediate reflow with this problem still in place would lose us any parallelism between the layout and content tasks on realistic workloads. IMO we need to figure out how to avoid having the content task block at any time on relayout, whether immediate or deferred.

@metajack
Copy link
Contributor

@metajack metajack commented May 13, 2013

Related to #424

@metajack
Copy link
Contributor

@metajack metajack commented May 13, 2013

This can be closed when #424 lands.

@jdm
Copy link
Member Author

@jdm jdm commented Feb 15, 2014

#424 landed.

@jdm jdm closed this Feb 15, 2014
ChrisParis pushed a commit to ChrisParis/servo that referenced this issue Sep 7, 2014
…anvas-drawCustomFocusRing-metadata

Added metadata to my previous test
glennw pushed a commit to glennw/servo that referenced this issue Jan 16, 2017
Add a number of changes to support running tests on OSMesa.

* Add a wrapper type for webgl contexts that allows selectinng
  between a native (hw accel) or osmesa (software) context at
  runtime.

* Change request_webgl_context behaviour to try creating a readback
  (non-shared) context if shared context creation fails. This means
  that the webgl code in servo doesn't have to be aware of whether
  it should be creating native or osmesa contexts in fallback mode.

* Flush each webgl context at the start of the frame to ensure
  contexts are painted correctly when sharing osmesa contexts.

* Add init option to specify what kind of renderering context
  is being used.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/417)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.