Use the uvar notifier pipe timestamp to avoid excessive polling

In the named pipe notifier, notifications are broadcast by writing to the
pipe, waiting briefly, and then reading it back. When clients see the pipe
as readable, they report the uvars as potentially changed and fish will
sync against the uvar file.

Prior to this change, we synced repeatedly when the pipe was readable. But
we can do somewhat better by also checking the named pipe's timestamp (via
fstat). If the pipe has not changed, then we can skip the sync even if
there is currently data lingering on it.

With this change we should sync against the variable file less often
(typically once or twice per write); in the next change we refactor this
logic so it's easier to follow.

src/env_universal_common.cpp

@@ -1323,6 +1323,7 @@ static autoclose_fd_t make_fifo(const wchar_t *test_path, const wchar_t *suffix)
 class universal_notifier_named_pipe_t final : public universal_notifier_t {
 #if !defined(__CYGWIN__)
     autoclose_fd_t pipe_fd;
+    file_id_t pipe_fd_id{};
     long long readback_time_usec{0};
     size_t readback_amount{0};
 
@@ -1344,6 +1345,19 @@ class universal_notifier_named_pipe_t final : public universal_notifier_t {
         ignore_result(read(pipe_fd.fd(), buff, sizeof buff));
     }
 
+    /// Check if the pipe's file ID (aka struct stat) is different from what we have stored.
+    /// If it has changed, it indicates that someone has written to the pipe; update our stored id.
+    /// \return true if changed, false if not.
+    bool check_pipe_id_changed() {
+        if (!pipe_fd.valid()) return false;
+        file_id_t file_id = file_id_for_fd(pipe_fd.fd());
+        if (file_id == this->pipe_fd_id) {
+            return false;
+        }
+        this->pipe_fd_id = file_id;
+        return true;
+    }
+
    public:
     explicit universal_notifier_named_pipe_t(const wchar_t *test_path)
         : pipe_fd(make_fifo(test_path, L".notifier")) {}
@@ -1373,6 +1387,8 @@ class universal_notifier_named_pipe_t final : public universal_notifier_t {
             polling_due_to_readable_fd = true;
             drain_if_still_readable_time_usec = get_time() + k_readable_too_long_duration_usec;
             should_sync = true;
+            // Update our pipe's timestamps.
+            check_pipe_id_changed();
         }
         return should_sync;
     }
@@ -1392,6 +1408,9 @@ class universal_notifier_named_pipe_t final : public universal_notifier_t {
         // Now schedule a read for some time in the future.
         this->readback_time_usec = get_time() + k_flash_duration_usec;
         this->readback_amount += sizeof c;
+
+        // No need to react to our own changes.
+        check_pipe_id_changed();
     }
 
     unsigned long usec_delay_between_polls() const override {
@@ -1458,7 +1477,7 @@ class universal_notifier_named_pipe_t final : public universal_notifier_t {
                 drain_excessive_data();
             }
         }
-        return true;
+        return check_pipe_id_changed();
     }
 #else  // this class isn't valid on this system
    public:

src/fish_tests.cpp

@@ -4002,27 +4002,10 @@ bool poll_notifier(const std::unique_ptr<universal_notifier_t> &note) {
     return result;
 }
 
-static void trigger_or_wait_for_notification(universal_notifier_t::notifier_strategy_t strategy) {
-    switch (strategy) {
-        case universal_notifier_t::strategy_shmem_polling: {
-            break;  // nothing required
-        }
-        case universal_notifier_t::strategy_notifyd: {
-            // notifyd requires a round trip to the notifyd server, which means we have to wait a
-            // little bit to receive it. In practice 40 ms seems to be enough.
-            usleep(40000);
-            break;
-        }
-        case universal_notifier_t::strategy_named_pipe: {
-            break;  // nothing required
-        }
-    }
-}
-
 static void test_notifiers_with_strategy(universal_notifier_t::notifier_strategy_t strategy) {
     say(L"Testing universal notifiers with strategy %d", (int)strategy);
-    std::unique_ptr<universal_notifier_t> notifiers[16];
-    size_t notifier_count = sizeof notifiers / sizeof *notifiers;
+    constexpr size_t notifier_count = 16;
+    std::unique_ptr<universal_notifier_t> notifiers[notifier_count];
 
     // Populate array of notifiers.
     for (size_t i = 0; i < notifier_count; i++) {
@@ -4041,21 +4024,31 @@ static void test_notifiers_with_strategy(universal_notifier_t::notifier_strategy
     for (size_t post_idx = 0; post_idx < notifier_count; post_idx++) {
         notifiers[post_idx]->post_notification();
 
-        // Do special stuff to "trigger" a notification for testing.
-        trigger_or_wait_for_notification(strategy);
+        if (strategy == universal_notifier_t::strategy_notifyd) {
+            // notifyd requires a round trip to the notifyd server, which means we have to wait a
+            // little bit to receive it. In practice 40 ms seems to be enough.
+            usleep(40000);
+        }
 
         for (size_t i = 0; i < notifier_count; i++) {
+            bool polled = poll_notifier(notifiers[i]);
+
             // We aren't concerned with the one who posted. Poll from it (to drain it), and then
             // skip it.
             if (i == post_idx) {
-                poll_notifier(notifiers[i]);
                 continue;
             }
 
-            if (!poll_notifier(notifiers[i])) {
+            if (!polled) {
                 err(L"Universal variable notifier (%lu) %p polled failed to notice changes, with "
                     L"strategy %d",
                     i, notifiers[i].get(), (int)strategy);
+                continue;
+            }
+            // It should not poll again immediately.
+            if (poll_notifier(notifiers[i])) {
+                err(L"Universal variable notifier (%lu) %p polled twice in a row with strategy %d",
+                    i, notifiers[i].get(), (int)strategy);
             }
         }
 

src/input_common.cpp

@@ -96,8 +96,8 @@ static readb_result_t readb(int in_fd, bool queue_is_empty) {
         // The priority order is: uvars, stdin, ioport.
         // Check to see if we want a universal variable barrier.
         // This may come about through readability, or through a call to poll().
-        if (notifier.poll() ||
-            (fdset.test(notifier_fd) && notifier.notification_fd_became_readable(notifier_fd))) {
+        if ((fdset.test(notifier_fd) && notifier.notification_fd_became_readable(notifier_fd)) ||
+            notifier.poll()) {
             if (env_universal_barrier() && !queue_is_empty) {
                 return readb_uvar_notified;
             }