[darcs-devel] darcs patch: Count number of times the --verbose and ... (and 4 more)

[David Roundy]

On Mon, Nov 05, 2007 at 09:45:00PM +0100, Eric Kow wrote:
> Here is an attempt at implementing a commute progress meter.  It lets
> you create an arbitrary number of named counters.  If you increment a
> counter, and it was the last counter incremented, it will write over the
> same line.  Otherwise, the counter will appear on a new line.
> 
> The code is just a proof of concept, so don't expect to see it in an
> official repository anytime soon.  Carges of amateurishness and
> crappiness in all forms shall be gratefully accepted, doubly so if
> you volunteer to fix the code.
>
> I suspect that the main reason we never implemented commute counters is
> perfectionist inertia: we wanted to do it right, so we never got it
> done. Maybe having a completely wrong implementation will help us to
> overcome this inertia.  That said, I cannot work on this right now, so
> please step right up!

Yes, this'd be a great project for someone! It's actually pretty easy in
terms of Deep Darcs Knowledge, but poses interesting challenges, and is
dead useful.

> Mon Nov  5 00:42:19 CET 2007  Eric Kow <eric.kow at gmail.com>
>   * Count number of times the --verbose and --quiet flags are passed in.
> 
> Mon Nov  5 00:43:30 CET 2007  Eric Kow <eric.kow at gmail.com>
>   * Add a new 'debugMode' global variable.
> 
> Mon Nov  5 00:46:11 CET 2007  Eric Kow <eric.kow at gmail.com>
>   * Set global debugMode flag on -v -v -v
> 
> Mon Nov  5 21:19:01 CET 2007  Eric Kow <eric.kow at gmail.com>
>   * Add some more debug functions.
>   
>   The debugCounter function lets you output named progress counters in
>   debug mode.
> 
> Mon Nov  5 21:25:54 CET 2007  Eric Kow <eric.kow at gmail.com>
>   * [issue72] Show commute progress.


> hunk ./src/Darcs/Global.lhs 95
> +debug :: String -> a -> a
> +debug msg x = if debugMode
> +                 then unsafePerformIO $ putStr msg >> return x
> +                 else x
> +
> +debugLn :: String -> a -> a
> +debugLn msg = debug (msg ++ "\n")

I'd vote for calling debugLn debug, and always adding the "\n".  Noone
really wants to add the newline themselves, and for debug formatting we
don't need extreme subtlety.

> +debugCounter :: String -> a -> a
> +debugCounter k x = if debugMode
> +                      then unsafePerformIO $ do
> +                           modifyIORef _debugCounters $ Map.insertWith (+) k 1

This code looks scary to me:  I suspect you have a memory leak and
stack-overflow bug waiting to happen.  The problem is that the (+) isn't
going to actually be evaluated until we read from the Map.

In this case, it looks like it's okay because we...

> +                           cs <- readIORef _debugCounters
> +                           previous <- readIORef _debugPrevious
> +                           writeIORef _debugPrevious k
> +                           if previous == k
> +                              then putChar '\r'
> +                              else putChar '\n'
> +                           putStr $ k ++ ": " ++ show (cs Map.! k) ++ ". "

...actually use the counter here.  But I'd rather have code that's more
robust... it's bad that removing this putStr introduces a memory leak.

I'm not sure how better to do this.  Ideally we'd have a strict version of
Data.Map, but we don't want to add new library dependencies.  There might
be a strict version of insertWith, but last time I asked there wasn't.
Basically, Data.Map is a horrible way to compute histograms (in spite of
the fact that it seems the perfect way to do so).

At a minimum, perhaps we could replace the modifyIORef with a read and
write, and use the data in between, so it's known to be real? And maybe
throw in a seq for good measure, in case someone removes the use?

Of course, it's probably never going to matter, since we won't be counting
too many things (famous last words?), but so often code gets copied and
pasted from one place to another, or modified beyond recognition, and I'd
rather not have this time bomb sitting there.

I'd vote for keeping track of which "event" we printed last, and using "^H"
to overwrite the number when possible.  That would save on real-estate
considerably, and might make this nice enough to use for non-debug
purposes, i.e. normal feedback.

The tracing you've added to commute looks fine (for debug purposes).  It
might be nice to have a compile flag that eliminates tracing entirely,
though, so we'd be able at least to easily test if the global variable
checks are hurting performance in the normal (non-debug) case.
-- 
David Roundy
Department of Physics
Oregon State University