Re: Bug? R: Org babel block execution *drastically* slower than in ESS session directly

[Eric Schulte <schulte.eric@gmail.com>]

Aaron Ecay <aaronecay@gmail.com> writes:

> 2012ko azaroak 16an, Eric Schulte-ek idatzi zuen:
>
>> 
>> The attached patch adds a "really-silent" results header argument.  To
>> see the impact compare the running time of the following two code
>> blocks.
>
> Unfortunately, the attached patch doesn’t work correctly.  This can be
> seen by replacing the “seq” command in your examples with a command that
> has side effects – notify-send, aplay, etc.  In the :results none” case,
> the command is not run at all.
>
> That’s because the “(funcall cmd body params)” call at l. 574 of ob.el
> (patched version) has been put in a branch that is only run if :results
> != none.  That funcall is responsible for actually evaluating the code
> block.
>

Oh!, thanks for catching this, I just pushed up a fix.

>
> (The indentation of the patch as applied isn’t correct – the two
> branches of the if on l. 565 are indented at the same depth as each
> other, and as the if.  So it’s possible that the problem is due to a
> paren in the wrong place.  But I cannot see a way to make this approach
> work.)
>

Yes, sometimes I find this approach to be preferable to make the diffs
more readable and to avoid over-indentation in very large functions.

>
> The code generating the slowdown is in backend-specific files.  So, for
> example, a new branch needs to be added to the case statements in
> org-babel-R-evaluate(-session), to detect the :results none case and not
> read the result.  I suspect that each backend will need this treatment
> (unless some of them share code).
>
> In the meantime, attached to this email is a patch that implements a
> size check on reading results.  If the results file is over 10kb, it
> asks the user whether to proceed.  You can test this by evaluating:
>
> #+begin_src sh :results silent
>   seq 10000
> #+end_src
>
> 10kb of results actually doesn’t result in a very serious hang (~5sec on
> my system).  But I chose the value as more of a sanity check – odds are
> (?) that very few people want to see 10k of results in the (mini)buffer.
> The value could be made customizable.
>
> I also chose the polarity of the y-or-n-p so that picking the default
> (yes) option does the sensible thing of not hanging emacs, although it
> thus does discard data.  I’m not sure which is the worse problem.
>

I may be outvoted, but I find this approach too be overly complicated.
Also, size may frequently not be the best proxy for the time which Emacs
will take to ingest the results.

Best,

>
> From 1053f3acfc21f24fc994ae85adff6779838b0ce7 Mon Sep 17 00:00:00 2001
> From: Aaron Ecay <aaronecay@gmail.com>
> Date: Sat, 17 Nov 2012 19:26:43 -0500
> Subject: [PATCH] lisp/ob.el: add a size check to
>  `org-babel-import-elisp-from-file'
>
> Reading large results can cause emacs to hang for a long time.  Ask the
> user whether to proceed in such cases.
>
> Signed-off-by: Aaron Ecay <aaronecay@gmail.com>
> ---
>  lisp/ob.el | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/lisp/ob.el b/lisp/ob.el
> index bf4b455..b2385e9 100644
> --- a/lisp/ob.el
> +++ b/lisp/ob.el
> @@ -2462,24 +2462,30 @@ appropriate."
>  (defun org-babel-import-elisp-from-file (file-name &optional separator)
>    "Read the results located at FILE-NAME into an elisp table.
>  If the table is trivial, then return it as a scalar."
> -  (let (result)
> -    (save-window-excursion
> -      (with-temp-buffer
> -	(condition-case err
> -	    (progn
> -	      (org-table-import file-name separator)
> -	      (delete-file file-name)
> -	      (setq result (mapcar (lambda (row)
> -				     (mapcar #'org-babel-string-read row))
> -				   (org-table-to-lisp))))
> -	  (error (message "Error reading results: %s" err) nil)))
> -      (if (null (cdr result)) ;; if result is trivial vector, then scalarize it
> -	  (if (consp (car result))
> -	      (if (null (cdr (car result)))
> -		  (caar result)
> -		result)
> -	    (car result))
> -	result))))
> +  (let* ((file-size (nth 7 (file-attributes file-name)))
> +         (can-load (or (< file-size (* 10 1024)) ; 10kb
> +                       (not (y-or-n-p (concat "Displaying the block's large "
> +                                              "results may hang emacs; skip "
> +                                              "reading them?"))))))
> +    (when can-load
> +      (let (result)
> +        (save-window-excursion
> +          (with-temp-buffer
> +            (condition-case err
> +                (progn
> +                  (org-table-import file-name separator)
> +                  (delete-file file-name)
> +                  (setq result (mapcar (lambda (row)
> +                                         (mapcar #'org-babel-string-read row))
> +                                       (org-table-to-lisp))))
> +              (error (message "Error reading results: %s" err) nil)))
> +          (if (null (cdr result)) ;; if result is trivial vector, then scalarize it
> +              (if (consp (car result))
> +                  (if (null (cdr (car result)))
> +                      (caar result)
> +                    result)
> +                (car result))
> +            result))))))
>  
>  (defun org-babel-string-read (cell)
>    "Strip nested \"s from around strings."
> -- 
> 1.8.0

-- 
Eric Schulte
http://cs.unm.edu/~eschulte