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

constellation: Simplify the scheduler so it doesn't create a thread for each event #11279

Closed
wants to merge 5 commits into from

Conversation

@emilio
Copy link
Member

emilio commented May 19, 2016

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

  • ./mach build -d does not report any errors
  • ./mach test-tidy --faster does not report any errors
  • These changes fix #11268

Either:

  • These changes do not require tests because it's 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 PR builds on top of #11272 (it couldn't be reopened because I pushed to the branch before doing it).

r? @asajeffrey

cc @metajack @pcwalton

Concretely:

@metajack: This uses an auxiliary thread to take care of timeouts. It's not optimal (optimal would be rust-lang/rfcs#962), but it's way better than anything we had, and the other option (passing the thread handle around and unpark() the thread when appropiate) wouldn't work across IPC.


This change is Reviewable

emilio added 3 commits May 19, 2016
…or each event

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.
This approach uses a secondary thread to manage the timeout + select.

This is not ideal, but it can be vastly improved if something is done about
rust-lang/rfcs#962.

The other apparently cleaner option, having the returned value not being an
ipc-sender, but a struct containing the sender and the Thread handle (which
would allow unparking), wouldn't work across IPC.
…o tasks
@highfive
Copy link

highfive commented May 19, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/timer_scheduler.rs
@asajeffrey
Copy link
Member

asajeffrey commented May 19, 2016

My completely untested implementation, for comparison: https://github.com/asajeffrey/servo/tree/scheduler-with-fewer-threads

@asajeffrey
Copy link
Member

asajeffrey commented May 19, 2016

Some IRC discussion: http://logs.glob.uno/?c=mozilla%23servo&s=19+May+2016&e=19+May+2016#c433870

TL;DR (but really it's very short): I'm concerned that this code may deadlock, or at least block for an unbounded amount of time, due to a race condition. Fixing it (as far as we can see) requires spinning on an atomic, since there's no test-and-park primitive.

@asajeffrey asajeffrey mentioned this pull request May 19, 2016
4 of 5 tasks complete
@asajeffrey
Copy link
Member

asajeffrey commented May 19, 2016

My now-tested implementation: #11283.

@emilio
Copy link
Member Author

emilio commented May 19, 2016

To be clear, I'm fine landing any of both approaches. I think this one would be a bit easier to migrate if rust-lang/rfcs#962 is implemented, but that's not a huge concern.

On the other side, after reading the docs more carefully, I think the discussion we had on IRC was wrong. This can't make us sleep too much. The point @asajeffrey was making was that if we called timeout_thread.unpark() before the thread started to park, it will sleep the timeout before returning.

From the docs:

park: Blocks unless or until the current thread's token is made available. 

(notice the "unless" bit).

That would mean that the worst case for this is the unlikely case that we call unpark() when the timeout thread's unpark_timeout has already ended, and thus the next time it would return automatically (since the token is available).

This is not a problem for this implementation, since once the timeout is raised, we'll re-check times before checking that again, and block, so this becomes a no-op.

In fact, I think this can't even be the case, since mpsc::recv() uses park() checking an atomic flag internally.

@asajeffrey
Copy link
Member

asajeffrey commented May 20, 2016

Ah, this makes life a lot easier, I'll try again with this semantics of park/unpark.

emilio added 2 commits May 20, 2016
This reverts commit a9879a8.
…on the

implementation of std::sync::mpsc::Sender.

Optimal, but not ideal. I have a PR in progress to implement recv_timeout()
@highfive
Copy link

highfive commented May 20, 2016

New code was committed to pull request.

@asajeffrey
Copy link
Member

asajeffrey commented May 20, 2016

We decided to go for #11283.

@asajeffrey asajeffrey closed this May 20, 2016
bors added a commit to rust-lang/rust that referenced this pull request Jun 21, 2016
std: sync: Implement recv_timeout()

This is an attempt to implement rust-lang/rfcs#962.

I'm not sure about if a change like this would require an rfc or something like
that, and this surely needs a lot more testing, but I wanted to take some eyes
on it before following.

cc @metajack @asajeffrey servo/servo#11279 servo/servo#11283

r? @aturon
bors added a commit to rust-lang/rust that referenced this pull request Jun 22, 2016
std: sync: Implement recv_timeout()

This is an attempt to implement rust-lang/rfcs#962.

I'm not sure about if a change like this would require an rfc or something like
that, and this surely needs a lot more testing, but I wanted to take some eyes
on it before following.

cc @metajack @asajeffrey servo/servo#11279 servo/servo#11283

r? @aturon
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.

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