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

Remove Access Control #194

Merged
merged 6 commits into from
Aug 29, 2020
Merged

Remove Access Control #194

merged 6 commits into from
Aug 29, 2020

Conversation

sunjay
Copy link
Owner

@sunjay sunjay commented Aug 28, 2020

The theme of my week seems to be "reduce and simplify". 🎉 😁

In #192, I took the AccessControl type and simplified it to remove all of the unnecessary task switching and other complexity that was introduced when it was first added in #173. While working on that PR, I realized that we could potentially remove AccessControl entirely and radically simplify our request handling all at once.

The key insights that lead to these changes are:

  • the borrowing rules guarantee that A) methods with &mut self can't be called concurrently with methods that are &self (&mut self and &self are mutually exclusive) and B) methods with &mut self can't be called concurrently with other methods that are &mut self (types like Mutex guarantee these properties when working with multiple threads)
  • all of the methods that cause an animation to occur (forward, right, go_to, etc.) are &mut self
  • all of the methods that get properties (pen_size, fill_color, etc.) are &self

All of this means that:

  1. only a single animation can be occurring at any given time for any given turtle
  2. properties will never be read while an animation is going on

Recall that the entire purpose of AccessControl was to guarantee that these two properties were upheld. While the current implementation robustly implements the IPC protocol to handle all cases, that really isn't necessary if the interface that the protocol is exposed from already enforces the correct behaviour.

Clearing The Drawing

The only change in behaviour is for the unstable and currently undocumented Drawing::clear method that is part of the multiple turtles feature (#16). Previously, because of AccessControls design, the clear method could not get access to all of the turtles until all the animations were complete. This led to the program "waiting" on turtles to finish drawing whatever line they were currently in the middle of.

This GIF demonstrates how the program pauses while it waits for the green line to finish so that it can clear the drawings:

Peek 2020-08-28 16-33

Without AccessControl, the behaviour of Drawing::clear changes to cancel all animations and stop the turtles wherever they were when clear was called.

This GIF shows the same program but with no pauses when clear is called:

Peek 2020-08-28 16-37

Notice that the pink and purple lines don't reach all the way to the right as they did in the previous gif. Since the clear call is in the middle of a line, that line gets cancelled, the turtle stops, and then resumes drawing its remaining lines from that interrupted position.

This behaviour change can be a bit subtle, but I think it's worth it to have simpler request handling code and better performance.

Implementation Notes

Instead of having separate locks for the drawing and for each turtle, we now have just a single lock for the entire App state. We are using synchronous locks from parking_lot since we don't need to await while holding on to the locks. Requests are all handled synchronously, in the order that they arrive. We no longer spawn a task per request.

A consequence of this new design is that animations can't run in the handlers themselves since that would pause all other request handling. Instead, we get concurrent turtle animations by sending each animation to an AnimationRunner task. That task drives each animation to completion.

The AnimationRunner task has to be careful to always end animations at exactly the right time. Any delays in that will cause the AnimationComplete message to not get sent in time which will delay the next line that a turtle was going to draw. To deal with this, the task keeps track of when each animation should be updated next. It updates each animation exactly when it needs to while trying to lock the state as little as possible. Each animation is updated regularly at 60 FPS or when it ends if that is prior to the next frame.

There don't appear to be any deadlocks in the loadtest anymore. This is probably because we now only have one lock over the app data and one lock over the display list. It's much easier to avoid deadlocks when you have fewer locks to order in the correct way!

@sunjay sunjay merged commit 4a06b0f into master Aug 29, 2020
@sunjay sunjay deleted the remove-access-control branch August 29, 2020 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant