Fix pty read sometimes not triggering draw

There was a lot of complexity around the threadsafe `Flag` type and
waking up the event loop. The idea was to prevent unnecessary calls to
the glutin window's wakeup_event_loop() method which can be expensive.
This complexity made it difficult to get synchronization between the pty
reader and the render thread correct. Now, the `dirty` flag on the
terminal is also used to prevent spurious wakeups. It is only changed
when the mutex is held, so race conditions associated with that flag
shouldn't happen.

7 files changed, 37 insertions, 103 deletions

diff --git a/src/display.rs b/src/display.rs
index e12d1c8f..8cf19f47 100644
--- a/src/display.rs
+++ b/src/display.rs
@@ -213,12 +213,6 @@ impl Display {
     ///
     /// This call may block if vsync is enabled
     pub fn draw(&mut self, mut terminal: MutexGuard<Term>, config: &Config, selection: &Selection) {
-        // This is a hack since sometimes we get stuck waiting for events
-        // in the main loop otherwise.
-        //
-        // TODO figure out why this is necessary
-        self.window.clear_wakeup_flag();
-
         // Clear dirty flag
         terminal.dirty = false;
 
diff --git a/src/event.rs b/src/event.rs
index c516f151..bfcb3f3c 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -105,7 +105,6 @@ impl<N: Notify> Processor<N> {
     fn handle_event<'a>(
         processor: &mut input::Processor<'a, N>,
         event: glutin::Event,
-        wakeup_request: &mut bool,
         ref_test: bool,
         resize_tx: &mpsc::Sender<(u32, u32)>,
     ) {
@@ -135,18 +134,14 @@ impl<N: Notify> Processor<N> {
             },
             glutin::Event::Resized(w, h) => {
                 resize_tx.send((w, h)).expect("send new size");
-
-                // Previously, this marked the terminal state as "dirty", but
-                // now the wakeup_request controls whether a display update is
-                // triggered.
-                *wakeup_request = true;
+                processor.ctx.terminal.dirty = true;
             },
             glutin::Event::KeyboardInput(state, _code, key, mods, string) => {
                 processor.process_key(state, key, mods, string);
             },
             glutin::Event::MouseInput(state, button) => {
                 processor.mouse_input(state, button);
-                *wakeup_request = true;
+                processor.ctx.terminal.dirty = true;
             },
             glutin::Event::MouseMoved(x, y) => {
                 let x = limit(x, 0, processor.ctx.size_info.width as i32);
@@ -155,17 +150,17 @@ impl<N: Notify> Processor<N> {
                 processor.mouse_moved(x as u32, y as u32);
 
                 if !processor.ctx.selection.is_empty() {
-                    *wakeup_request = true;
+                    processor.ctx.terminal.dirty = true;
                 }
             },
             glutin::Event::Focused(true) => {
-                *wakeup_request = true;
+                processor.ctx.terminal.dirty = true;
             },
             glutin::Event::MouseWheel(scroll_delta, touch_phase) => {
                 processor.on_mouse_wheel(scroll_delta, touch_phase);
             },
             glutin::Event::Awakened => {
-                *wakeup_request = true;
+                processor.ctx.terminal.dirty = true;
             },
             _ => (),
         }
@@ -176,13 +171,11 @@ impl<N: Notify> Processor<N> {
         &mut self,
         term: &'a FairMutex<Term>,
         window: &Window
-    ) -> (MutexGuard<'a, Term>, bool) {
-        let mut wakeup_request = false;
-
+    ) -> MutexGuard<'a, Term> {
         // Terminal is lazily initialized the first time an event is returned
         // from the blocking WaitEventsIterator. Otherwise, the pty reader would
         // be blocked the entire time we wait for input!
-        let terminal;
+        let mut terminal;
 
         {
             // Ditto on lazy initialization for context and processor.
@@ -195,7 +188,6 @@ impl<N: Notify> Processor<N> {
                     Processor::handle_event(
                         &mut processor,
                         $event,
-                        &mut wakeup_request,
                         self.ref_test,
                         &self.resize_tx,
                     )
@@ -206,7 +198,7 @@ impl<N: Notify> Processor<N> {
                 Some(event) => {
                     terminal = term.lock();
                     context = ActionContext {
-                        terminal: &terminal,
+                        terminal: &mut terminal,
                         notifier: &mut self.notifier,
                         selection: &mut self.selection,
                         mouse: &mut self.mouse,
@@ -230,7 +222,7 @@ impl<N: Notify> Processor<N> {
             }
         }
 
-        (terminal, wakeup_request)
+        terminal
     }
 
     pub fn update_config(&mut self, config: &Config) {
diff --git a/src/event_loop.rs b/src/event_loop.rs
index a6eed40e..d1c52367 100644
--- a/src/event_loop.rs
+++ b/src/event_loop.rs
@@ -211,9 +211,14 @@ impl<Io> EventLoop<Io>
                         state.parser.advance(&mut *terminal, *byte, &mut self.pty);
                     }
 
-                    terminal.dirty = true;
-
-                    self.display.notify();
+                    // Only request a draw if one hasn't already been requested.
+                    //
+                    // This is a performance optimization even if only for X11
+                    // which is very expensive to hammer on the even loop wakeup
+                    if !terminal.dirty {
+                        self.display.notify();
+                        terminal.dirty = true;
+                    }
                 },
                 Err(err) => {
                     match err.kind() {
diff --git a/src/input.rs b/src/input.rs
index 84ed86ec..60183098 100644
--- a/src/input.rs
+++ b/src/input.rs
@@ -49,7 +49,7 @@ pub struct Processor<'a, N: 'a> {
 
 pub struct ActionContext<'a, N: 'a> {
     pub notifier: &'a mut N,
-    pub terminal: &'a Term,
+    pub terminal: &'a mut Term,
     pub selection: &'a mut Selection,
     pub mouse: &'a mut Mouse,
     pub size_info: &'a term::SizeInfo,
diff --git a/src/main.rs b/src/main.rs
index 0f89aa06..cce55354 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -79,7 +79,8 @@ fn run(mut config: Config, options: cli::Options) -> Result<(), Box<Error>> {
     // This object contains all of the state about what's being displayed. It's
     // wrapped in a clonable mutex since both the I/O loop and display need to
     // access it.
-    let terminal = Arc::new(FairMutex::new(Term::new(display.size().to_owned())));
+    let terminal = Term::new(display.size().to_owned());
+    let terminal = Arc::new(FairMutex::new(terminal));
 
     // Create the pty
     //
@@ -129,20 +130,20 @@ fn run(mut config: Config, options: cli::Options) -> Result<(), Box<Error>> {
     // Main display loop
     loop {
         // Process input and window events
-        let (mut terminal, wakeup_request) = processor.process_events(&terminal, display.window());
+        let mut terminal = processor.process_events(&terminal, display.window());
 
         // Handle config reloads
-        let config_updated = config_monitor.as_ref()
+        config_monitor.as_ref()
             .and_then(|monitor| monitor.pending_config())
             .map(|new_config| {
                 config = new_config;
                 display.update_config(&config);
                 processor.update_config(&config);
-                true
-            }).unwrap_or(false);
+                terminal.dirty = true;
+            });
 
         // Maybe draw the terminal
-        if wakeup_request || config_updated {
+        if terminal.needs_draw() {
             // Handle pending resize events
             //
             // The second argument is a list of types that want to be notified
diff --git a/src/term/mod.rs b/src/term/mod.rs
index 29bf6b83..4e4a022c 100644
--- a/src/term/mod.rs
+++ b/src/term/mod.rs
@@ -298,7 +298,7 @@ impl Term {
         let scroll_region = Line(0)..grid.num_lines();
 
         Term {
-            dirty: true,
+            dirty: false,
             grid: grid,
             alt_grid: alt,
             alt: false,
@@ -313,6 +313,11 @@ impl Term {
         }
     }
 
+    #[inline]
+    pub fn needs_draw(&self) -> bool {
+        self.dirty
+    }
+
     pub fn string_from_selection(&self, span: &Span) -> String {
         /// Need a generic push() for the Append trait
         trait PushChar {
diff --git a/src/window.rs b/src/window.rs
index 62c65c9c..edf1e7f1 100644
--- a/src/window.rs
+++ b/src/window.rs
@@ -11,8 +11,6 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-use std::sync::Arc;
-use std::sync::atomic::{AtomicBool, Ordering};
 use std::convert::From;
 use std::fmt::{self, Display};
 use std::ops::Deref;
@@ -57,16 +55,11 @@ type Result<T> = ::std::result::Result<T, Error>;
 /// Wraps the underlying windowing library to provide a stable API in Alacritty
 pub struct Window {
     glutin_window: glutin::Window,
-
-    /// This flag allows calls to wakeup_event_loop to be coalesced. Waking the
-    /// event loop is potentially very slow (and indeed is on X11).
-    flag: Flag
 }
 
 /// Threadsafe APIs for the window
 pub struct Proxy {
     inner: glutin::WindowProxy,
-    flag: Flag,
 }
 
 /// Information about where the window is being displayed
@@ -98,46 +91,6 @@ pub struct Pixels<T>(pub T);
 #[derive(Debug, Copy, Clone)]
 pub struct Points<T>(pub T);
 
-/// A wrapper around glutin's `WaitEventsIterator` that clears the wakeup
-/// optimization flag on drop.
-pub struct WaitEventsIterator<'a> {
-    inner: glutin::WaitEventsIterator<'a>,
-    flag: Flag
-}
-
-impl<'a> Iterator for WaitEventsIterator<'a> {
-    type Item = glutin::Event;
-
-    #[inline]
-    fn next(&mut self) -> Option<Self::Item> {
-        self.inner.next()
-    }
-}
-
-impl<'a> Drop for WaitEventsIterator<'a> {
-    fn drop(&mut self) {
-        self.flag.set(false);
-    }
-}
-
-#[derive(Clone)]
-pub struct Flag(pub Arc<AtomicBool>);
-impl Flag {
-    pub fn new(initial_value: bool) -> Flag {
-        Flag(Arc::new(AtomicBool::new(initial_value)))
-    }
-
-    #[inline]
-    pub fn get(&self) -> bool {
-        self.0.load(Ordering::Acquire)
-    }
-
-    #[inline]
-    pub fn set(&self, value: bool) {
-        self.0.store(value, Ordering::Release)
-    }
-}
-
 pub trait ToPoints {
     fn to_points(&self, scale: f32) -> Size<Points<u32>>;
 }
@@ -265,7 +218,6 @@ impl Window {
 
         Ok(Window {
             glutin_window: window,
-            flag: Flag::new(false),
         })
     }
 
@@ -308,7 +260,6 @@ impl Window {
     pub fn create_window_proxy(&self) -> Proxy {
         Proxy {
             inner: self.glutin_window.create_window_proxy(),
-            flag: self.flag.clone(),
         }
     }
 
@@ -319,26 +270,18 @@ impl Window {
             .map_err(From::from)
     }
 
-    /// Block waiting for events
+    /// Poll for any available events
     #[inline]
-    pub fn wait_events(&self) -> WaitEventsIterator {
-        WaitEventsIterator {
-            inner: self.glutin_window.wait_events(),
-            flag: self.flag.clone()
-        }
-    }
-
-    /// Clear the wakeup optimization flag
-    pub fn clear_wakeup_flag(&self) {
-        self.flag.set(false);
+    pub fn poll_events(&self) -> glutin::PollEventsIterator {
+        self.glutin_window.poll_events()
     }
 
     /// Block waiting for events
     ///
     /// FIXME should return our own type
     #[inline]
-    pub fn poll_events(&self) -> glutin::PollEventsIterator {
-        self.glutin_window.poll_events()
+    pub fn wait_events(&self) -> glutin::WaitEventsIterator {
+        self.glutin_window.wait_events()
     }
 }
 
@@ -348,13 +291,7 @@ impl Proxy {
     /// This is useful for triggering a draw when the renderer would otherwise
     /// be waiting on user input.
     pub fn wakeup_event_loop(&self) {
-        // Only wake up the window event loop if it hasn't already been
-        // signaled. This is a really important optimization because waking up
-        // the event loop redundantly burns *a lot* of cycles.
-        if !self.flag.get() {
-            self.inner.wakeup_event_loop();
-            self.flag.set(true);
-        }
+        self.inner.wakeup_event_loop();
     }
 }