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

Avoid creating a thread every single event we receive #11268

Closed
emilio opened this issue May 19, 2016 · 1 comment
Closed

Avoid creating a thread every single event we receive #11268

emilio opened this issue May 19, 2016 · 1 comment

Comments

@emilio
Copy link
Member

@emilio emilio commented May 19, 2016

We're doing this right now, for some reason I can't understand. We spawn a single thread just to sleep, ensure we're not cancelled, and send an empty message.

If we're going to use threads, we should at least use a thread pool, but probably there's a smarter way to do this, like a single thread that sleeps just the necessary time to dispatch the next event, or something.

I plan to take a look at this, but my time is actually constrained a lot, so if someone wants to investigate and refactor this, please do!

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented May 19, 2016

I think I can see how to do this fairly easily: we have a dedicated thread for the event queue, which parks itself, then when it is unparked looks to see if the head of the queue is in the past, and if so pops and triggers it, then goes back to being parked. To schedule an event, we just add it to the event queue (in time order) and if we added to the front of the queue, unpark the queue thread.

Should be a reasonably straightforward use of thread parking and a priority queue, the tricky bit is going to be understanding the current API and if anything depends on the current behaviour.

bors-servo added a commit that referenced this issue May 19, 2016
[DO NOT MERGE] constellation: Simplify the scheduler so it doesn't create a thread for each event

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy --faster` does not report any errors
- [x] These changes fix #11268 (github issue number if applicable).

Either:
- [x] These changes do not require tests because this is a refactoring

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

---

This can't land as-is, just wanted to make sure this idea is fine, and do a try
run because I need to leave now.

r? @asajeffrey

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11272)
<!-- Reviewable:end -->
@asajeffrey asajeffrey mentioned this issue May 19, 2016
4 of 5 tasks complete
bors-servo added a commit that referenced this issue May 20, 2016
Scheduler with fewer threads

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy --faster` does not report any errors
- [X] These changes fix #11268.

Either:
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it's improving perf, not adding functionality.

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11283)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 20, 2016
Scheduler with fewer threads

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy --faster` does not report any errors
- [X] These changes fix #11268.

Either:
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it's improving perf, not adding functionality.

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11283)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.