Expensive ToString and property getters - bad for debugging and against .NET design guidelines

Mar 7, 2010 at 5:56 AM

Hi,

I'm loading some large (but not huge) sequences into a Sequence (for example the 250MBP human chromosome 1).  This is small enough that I want to work with it all in memory (rather than use a virtual sequence that reads chunks from the disk) - 125MB is no problem on my 4GB machine.  However, debugging my application when it's using sequences of this size is rather painful.  For example, VS automatically invokes Sequence.ToString on every step whenever a sequence is visible in my autos/locals/watch window - clearly copying and expanding the sequence to a string automatically is not what I want.  VS hangs for 5-10 seconds on each step, then the func-eval times out and all function evaluation is disabled until I step again (which makes debugging very difficult).  There are also some properties (which VS evaluates by default) which are expensive and cause timeouts, for example Sequence.Statistics.

So this leads me to turn off the "enable automatic property evaluation and other implicit function calls" option whenever debugging with non-trivial sequences.  This is a pain - it's nice having automatic evaluation for the simple properties and annoying to have to remember to do this when working just with MBF (since it's a global setting, not per-project).

The .NET Framework design guidelines say "Properties are used like fields, meaning that properties should not be computationally complex or produce side effects.", and (in the 2nd edition) "DO try to keep the string returned from ToString short" (details here).  Since sequences may be large, I'd argue that no properties or ToString methods should iterate over an entire sequence (such operations should be done on other methods).  I guess there's probably a bunch of code out there already that relies on the Sequence.ToString() behavior, but perhaps it's best to change it now (eg. maybe output a short prefix, with another method to convert the whole thing to a string explicitly if that's really what someone wants) while MBF is still in beta.  Alternately you may be able to use a DebuggerDisplayAttribute to work around the problem (I think it can be used to prevent the ToString from being called) - but it's still really a violation of the guidelines and could cause trouble elsewhere, it really seems like such a potentially expensive operation deserves it's own name that someone should request explicitly rather than overloading an existing commonly-used API.

Thanks!
   Rick

Coordinator
Mar 10, 2010 at 1:19 AM

Great feedback Rick!  If you haven't already done so, please log a work item on this issue and we'll get this on the backlog.  Alternatively, we'd be happy to see you contribute a solution back to the project to address the issue as well:).  We're working to better define the process of contributing code, but for now are expecting folks to adhere to our coding standards guide (soon to be released) and to submit changes for review prior to committing the code.

I'll be sure to update the Documentation section with more details as we flesh out the process, but I'm super excited to see your interest and well-thought out description of how to imporove the code.

Mar 16, 2010 at 6:39 AM

Thanks Mike! 

I'll code up a suggested change (including updating any tests that fail) and then file a work-item with a patch!

Rick