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

Debug servo build is intermittently unresponsive #22988

Closed
jdm opened this issue Mar 6, 2019 · 6 comments
Closed

Debug servo build is intermittently unresponsive #22988

jdm opened this issue Mar 6, 2019 · 6 comments
Labels

Comments

@jdm
Copy link
Member

@jdm jdm commented Mar 6, 2019

When I load https://threejs.org/examples in a debug servo build, often it works as expected. Once in a while it takes a very long time before the hover styles start appearing under the mouse. There is no reported hang from the background hang monitor.

@jdm jdm added the I-perf-slow label Mar 6, 2019
@jdm
Copy link
Member Author

@jdm jdm commented Mar 7, 2019

Judging by RUST_LOG=script::scipt_thread, it spends a really long time processing a particular network response event.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 7, 2019

It appears to be hanging while reflowing after the DOMContentLoaded event, with a layout that is taking a ridiculously long time.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 7, 2019

Servo appears to be issuing a reflow after every single DOM node that is parsed. This is terrible!

@jdm
Copy link
Member Author

@jdm jdm commented Mar 7, 2019

Ok, there are a bunch of problems conspiring to make this really bad:

  • we unnecessarily perform a synchronous reflow in Document::maybe_dispatch_dom_content_loaded
  • the page is using the innerHTML setter, which creates a document fragment and a new document which has a pointer to the original element's window
  • the page uses innerHTML on every single link in the list of examples

Taken together this is what happens - for each link in the list of examples, we create a new document, parse a bit of text into it, finish parsing and fire a DOMContentLoadedEvent at the new document, then synchronously reflow the main page without having made any changes to it. Also the synchronous reflow can't be short-circuited because we insist on dirtying every node in the page.

@jdm jdm mentioned this issue Mar 7, 2019
4 of 4 tasks complete
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Mar 7, 2019

Possibly related? #22926

This sometimes shows itself in the log as a network hang, but it's actually waiting for window events.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 7, 2019

This issue reflects a particular edge case involving lots of uses of innerHTML during initial document load. I wouldn't assume too broad an impact.

bors-servo added a commit that referenced this issue Mar 8, 2019
Remove an unnecessary synchronous full reflow.

We already reflow any nodes that are dirtied during any turn of the event loop. There is no reason to synchronously reflow the entire document, especially when we don't even modify it in this method.

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22988
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22995)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 8, 2019
Remove an unnecessary synchronous full reflow.

We already reflow any nodes that are dirtied during any turn of the event loop. There is no reason to synchronously reflow the entire document, especially when we don't even modify it in this method.

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22988
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22995)
<!-- 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.

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