[GitHub] [systemds] mboehm7 edited a comment on pull request #984: [SYSTEMDS-2540] Stop Exception and Test improvements

[GitBox <gi...@apache.org>]

mboehm7 edited a comment on pull request #984:
URL: https://github.com/apache/systemds/pull/984#issuecomment-663911394


   Thanks for the initiative @Baunsgaard. Some of these changes are very good, on others I'm kind of split. I would recommend we merge it in (with the changes I made) and see how it works out - if it doesn't we change the defaults accordingly.
   
   * Logging: I changed some of the logging, especially in `VariableCPInstruction` (which makes a substantial portion of all execution instructions) to guard any logging of concatenated messages behind `if( LOG.isDebugEnabled() )`. Yes, the `LOG.debug` would discard the message, but cannot (without inlining) avoid the string concatenation. With scalar and small-matrix operations this logging would add significant overhead.
   
   * Test Output Buffering: The new output buffering is useful for some scripts that want to check the output. However, for scripts with large output, this poses a scalability bottleneck (collect all output instead of streaming print & discard), developers have to wait for the output until a test finishes, and we might not see certain problems during development (e.g., remaining development debug output). I now added a method `setOutputBuffering(false)` to disable this output buffering. We go with your new default and let people disable it during development. If this does not work for the team, we simply change the default.
   
   * Rewrite Errors: The constant folding rewrite (applied on compilation and dynamic recompilation during runtime) has been change from reporting errors and continuing execution to a hard crash. Meanwhile this is fine as these rewrites are stable enough and our exceptions have been changed to subclassing `RuntimeException`, so we can leave it like that. 
   
   * Error Handling: Stop and parser issues were on purpose not reported as exceptions but reported in a more user-friendly manner. Now, that we propagate everything as exceptions, a run from command line would throw potentially very deep stacktraces for these. I'm not sure this is a good idea as it simplifies development and testing at the expense of our users. Again, let's give it a try - if it turns out to be unacceptable, we change it to two slightly different entry points for tests and command line invocations. 
   
   * Minor Issues: Furthermore, I removed some unnecessary imports, changed instance methods to static methods where possible, and replaced some loops with more succinct lambda expressions. Regarding formatting, please avoid `} else`. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org