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

Improved headless Servo performance #18777

Merged
merged 2 commits into from Oct 10, 2017

Conversation

@mateon1
Copy link
Contributor

mateon1 commented Oct 7, 2017

Now the main thread doesn't waste 100% CPU, polling for events.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #18770 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because: I verified Servo works correctly in headless mode, and the linked issue is gone.

This change is Reviewable

Now the main thread doesn't waste 100% CPU
@highfive
Copy link

highfive commented Oct 7, 2017

Heads up! This PR modifies the following files:

@paulrouget
Copy link
Contributor

paulrouget commented Oct 8, 2017

This is good enough for tests. If headless mode is only used for tests, I think this approach is ok.

But if Headless mode is used for anything else, we need a better approach. I think the proper way of doing this is:

  • In Window::wait_event , for Headless, where non-headless do: window.poll_events(), we should block (park the main thread)
  • in create_window_proxy, for Headless, implement a thread-safe object that will wake up the blocked thread

Then the loop would run only if needed.

@nox
Copy link
Member

nox commented Oct 9, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2017

Trying commit 35b7527 with merge 40014c1...

bors-servo added a commit that referenced this pull request Oct 9, 2017
Improved headless Servo performance

Now the main thread doesn't waste 100% CPU, polling for events.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #18770 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because: I verified Servo works correctly in headless mode, and the linked issue is gone.

<!-- 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/18777)
<!-- Reviewable:end -->
@nox nox assigned paulrouget and unassigned nox Oct 9, 2017
@paulrouget
Copy link
Contributor

paulrouget commented Oct 9, 2017

@jdm do you think we should just add this sleep call, or do as I describe in my previous comment (less trivial)?

I'm happy with both approaches.

@jdm
Copy link
Member

jdm commented Oct 9, 2017

Why don't we make this change right now and file an issue for doing it the proper way?

@paulrouget
Copy link
Contributor

paulrouget commented Oct 9, 2017

Ok.

@mateon1: code looks good. Maybe move the sleep in WindowKind::Headless(..) => {} and add a comment.

@paulrouget
Copy link
Contributor

paulrouget commented Oct 9, 2017

Thank you.

I filed #18799

Can someone r+ this?

@jdm
Copy link
Member

jdm commented Oct 9, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2017

📌 Commit 63d69db has been approved by jdm

@highfive highfive assigned jdm and unassigned paulrouget Oct 9, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2017

Testing commit 63d69db with merge 54f6e87...

bors-servo added a commit that referenced this pull request Oct 9, 2017
Improved headless Servo performance

Now the main thread doesn't waste 100% CPU, polling for events.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #18770 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because: I verified Servo works correctly in headless mode, and the linked issue is gone.

<!-- 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/18777)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 10, 2017

@bors-servo bors-servo merged commit 63d69db into servo:master Oct 10, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mateon1 mateon1 deleted the mateon1:perf/headless-event-poll branch Oct 10, 2017
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.

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