Skip to content
This repository has been archived by the owner. It is now read-only.

Optimising Monitors #453

Closed
psibi opened this issue Jun 1, 2020 · 9 comments
Closed

Optimising Monitors #453

psibi opened this issue Jun 1, 2020 · 9 comments

Comments

@psibi
Copy link
Contributor

psibi commented Jun 1, 2020

Sorry for creating a bunch of issue, this is the last one.

Right now the way the Monitors are developed is that - we are coupling both the parsing and the formatting closely together. This is kind of inefficent right now. For example, in the CPU Monitor - I can see that we are formatting all the 10 values even if in the template I'm just using a single value out of it. We need to be more smart and do less computation when it's not required. I'm currently using a forked version of CPU monitor where I format only the required stuff and I can see that my xmobar's CPU consumption has reduced somewhat.

I'm planning to implement the de-coupling part but would like to know if others have any opinions/objections on it.

@slotThe
Copy link
Contributor

slotThe commented Jun 6, 2020

This sounds good to me. I guess separating these two things is more natural than doing them all at once all the time anyways.

Another thing I've noticed a while back is that we are currently parsing all of the arguments every time a monitor is executed (by calling Xmobar.Plugins.Monitors.Run.doArgs every time we're running the respective reader). While the config fields are not very large and so this is probably not an issue most of the time, it may conceivably have an effect on monitors that have a very fast update rate (probably in the range of < 1 seconds).

@jaor
Copy link
Owner

jaor commented Jun 7, 2020

@psibi
Copy link
Contributor Author

psibi commented Jun 8, 2020

In the current approach, note that the same code is being executed each and every time. So I don't think we are taking any advantage of laziness.

so i'd only change that if we find a way that keeps the code simple

I agree. I have a private working branch in my xmobar's fork where I have a prototype version where I have split those parts. IMO, the code is still simple. I'm thinking to do some perf analysis to see what benefit does it bring before actually proposing an MR here.

@slotThe
Copy link
Contributor

slotThe commented Jun 9, 2020

@psibi
Copy link
Contributor Author

psibi commented Jun 10, 2020

@slotThe Where is the attached patch ? I would be likely working on this as part of ZuriHac and would like to see it before I start working or adapting your code.

@slotThe
Copy link
Contributor

slotThe commented Jun 10, 2020

@slotThe Where is the attached patch ?

Ah, it must have swallowed it completely!

diff --git a/src/Xmobar/Plugins/Monitors/Common/Run.hs b/src/Xmobar/Plugins/Monitors/Common/Run.hs
index 3baa7aa..9709738 100644
--- a/src/Xmobar/Plugins/Monitors/Common/Run.hs
+++ b/src/Xmobar/Plugins/Monitors/Common/Run.hs
@@ -1,3 +1,5 @@
+{-# LANGUAGE BangPatterns #-}
+{-# LANGUAGE LambdaCase   #-}
 ------------------------------------------------------------------------------
 -- |
 -- Module: Xmobar.Plugins.Monitors.Run
@@ -25,6 +27,7 @@ module Xmobar.Plugins.Monitors.Common.Run ( runM
                                           ) where
 
 import Control.Exception (SomeException,handle)
+import Data.Functor (($>))
 import Data.List
 import Control.Monad.Reader
 import System.Console.GetOpt
@@ -65,18 +68,19 @@ getArgvs args =
         (_, n, []  ) -> n
         (_, _, errs) -> errs
 
-doArgs :: [String]
+parseArgs :: [String] -> Monitor (Either String [String])
+parseArgs args = case getOpt Permute options args of
+    (o, n, [])   -> doConfigOptions o $> Right n
+    (_, _, errs) -> pure $ Left (concat errs)
+
+doMon :: Monitor (Either String [String])
        -> ([String] -> Monitor String)
        -> ([String] -> Monitor Bool)
        -> Monitor String
-doArgs args action detect =
-    case getOpt Permute options args of
-      (o, n, [])   -> do doConfigOptions o
-                         ready <- detect n
-                         if ready
-                            then action n
-                            else return "<Waiting...>"
-      (_, _, errs) -> return (concat errs)
+doMon monitor action detect = monitor >>= \case
+    Left errs -> pure errs
+    Right n   -> do ready <- detect n
+                    if ready then action n else pure "<Waiting...>"
 
 doConfigOptions :: [Opts] -> Monitor ()
 doConfigOptions [] = io $ return ()
@@ -122,9 +126,10 @@ runMB args conf action wait = runMBD args conf action wait (\_ -> return True)
 
 runMBD :: [String] -> IO MConfig -> ([String] -> Monitor String) -> IO ()
         -> ([String] -> Monitor Bool) -> (String -> IO ()) -> IO ()
-runMBD args conf action wait detect cb = handle (cb . showException) loop
-  where ac = doArgs args action detect
-        loop = conf >>= runReaderT ac >>= cb >> wait >> loop
+runMBD args conf action wait detect cb = handle (cb . showException) (loop ac)
+  where !monitorConf = parseArgs args
+        ac = doMon monitorConf action detect
+        loop mon = conf >>= runReaderT mon >>= cb >> wait >> loop mon
 
 runML :: [String] -> IO MConfig -> ([String] -> Monitor String)
       -> (IO () -> IO ()) -> (String -> IO ()) -> IO ()
@@ -133,9 +138,10 @@ runML args conf action looper = runMLD args conf action looper (\_ -> return Tru
 runMLD :: [String] -> IO MConfig -> ([String] -> Monitor String)
        -> (IO () -> IO ()) -> ([String] -> Monitor Bool) -> (String -> IO ())
        -> IO ()
-runMLD args conf action looper detect cb = handle (cb . showException) loop
-  where ac = doArgs args action detect
-        loop = looper $ conf >>= runReaderT ac >>= cb
+runMLD args conf action looper detect cb = handle (cb . showException) (loop ac)
+  where !monitorConf = parseArgs args
+        ac = doMon monitorConf action detect
+        loop mon = looper $ conf >>= runReaderT mon >>= cb
 
 showException :: SomeException -> String
 showException = ("error: "++) . show . flip asTypeOf undefined

@psibi
Copy link
Contributor Author

psibi commented Jun 10, 2020

@slotThe Nice. Thanks for sharing! Note that in case you are interested - I have similar but quite different version of this in my branch: https://github.com/psibi/xmobar/tree/optimize-weather-cpu

@psibi
Copy link
Contributor Author

psibi commented Jun 13, 2020

@jaor / @slotThe I decoupled the parsing and the formatting part. And I get a perf boost of 12% via that for the CPU plugin. Since the change is huge - my plan is to create a PR only for the CPU monitor. My plan is slowly to decouple other plugins also and remove the whole MConfig type (and have a pure variant instead) - but for right now this seems to be more pragmatic.

@jaor
Copy link
Owner

jaor commented Jul 10, 2022

@jaor jaor closed this as completed Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants