Re: [patch suggestion] Mitigating the poor Emacs performance on huge org files: Do not use overlays for PROPERTY and LOGBOOK drawers

[Ihor Radchenko <yantar92@gmail.com>]

Note that the following commits seems to break my patch:

074ea1629 origin/master master Deprecate `org-cycle-hide-drawers'
1027e0256 Implement `org-cycle-hide-property-drawers'
8b05c06d4 Use `outline' invisibility spec for property drawers

The patch should work for commit ed0e75d24 in master.

Best,
Ihor


Ihor Radchenko <yantar92@gmail.com> writes:

> I have prepared a patch taking into account your comments and fixing
> other issues, reported by Karl Voit and found by myself.
>
> Summary of what is done in the patch:
>
> 1. iSearching in drawers is rewritten using using
> isearch-filter-predicate and isearch-mode-end-hook.
> The idea is to create temporary overlays in place of drawers to make
> isearch work as usual.
>
> 2. Change org-show-set-visibility to consider text properties. This
> makes helm-occur open drawers.
>
> 3. Make sure (partially) that text inserted into hidden drawers is also
> hidden (to avoid glitches reported by Karl Voit).
> The reason why it was happening was because `insert' does not inherit
> text properties by default, which means that all the inserted text is
> visible by default. I have changes some instances of insert and
> insert-before-markers to thair *-and-inherit versions. Still looking
> into where else I need to do the replacement.
>
> Note that "glitch" might appear in many external packages writing into
> org drawers. I do not think that insert-and-inherit is often used or
> even known.
>
> Remaining problems:
>
> 1. insert-* -> insert-*-and-inherit replacement will at least need to be
> done in org-table.el and probably other places
>
> 2. I found hi-lock re-opening drawers after exiting isearch for some
> reason. This happens when hi-lock tries to highlight isearch matches.
> Not sure about the cause.
>
> 3. There is still some visual glitch when unnecessary org-ellipsis is
> shown when text was inserted into hidden property drawer, though the
> inserted text itself is hidden. 
>
>>> (defun org-find-text-property-region (pos prop)
>>>   "Find a region containing PROP text property around point POS."
>>>   (require 'org-macs) ;; org-with-point-at
>>>   (org-with-point-at pos
>>
>> Do we really need that since every function has a POS argument anyway?
>> Is it for the `widen' part?
>
> Yes, it is not needed. Fixed.
>
>>>     (let* ((beg (and (get-text-property pos prop) pos))
>>> 	   (end beg))
>>>       (when beg
>>> 	(setq beg (or (previous-single-property-change pos prop)
>>> 		      beg))
>>
>> Shouldn't fall-back be (point-min)?
>>
>>> 	(setq end (or (next-single-property-change pos prop)
>>> 		      end))
>>
>> And (point-max) here?
>
> No, (point-min) and (point-max) may cause problems there.
> previous/next-single-property-change returns nil when called at the
> beginning/end of the region with given text property. Falling back to
> (point-min/max) may wrongly return too large region.
>
>> Nitpick: `equal' -> =
>
> Fixed.
>
>> Or, it seems nicer to `add-function' around `isearch-filter-predicate'
>> and extend isearch-filter-visible to support (i.e., stop at, and
>> display) invisible text through text properties.
>
> Done. I used
> (setq-local isearch-filter-predicate #'org--isearch-filter-predicate),
> which should be even cleaner. 
>
>> I wonder how it compares to drawers using the same invisible spec as
>> headlines, as it was the case before. Could you give it a try? 
>
>> I think hiding all property drawers right after opening a subtree is
>> fast enough.
>
> I am not sure what you refer to. Just saw your relevant commit. Will
> test ASAP.
>
> Without testing, the code does not seem to change the number of
> overlays. A new overlay is still created for each property drawer.
> As I mentioned in the first email, the large number of overlays is what
> makes Emacs slow. Citing Eli Zaretskii's reply to my Bug#354553,
> explaining why Emacs becomes slow on large org file:
>
> "... When C-n calls vertical-motion, the latter needs to find the
> buffer position displayed directly below the place where you typed
> C-n.  Since much of the text between these places, vertical-motion
> needs to skip the invisible text as quickly as possible, because from
> the user's POV that text "doesn't exist": it isn't on the screen.
> However, Org makes this skipping exceedingly hard, because (1) it uses
> overlays (as opposed to text properties) to hide text, and (2) it puts
> an awful lot of overlays on the hidden text: there are 18400 overlays
> in this file's buffer, 17500 of them between the 3rd and the 4th
> heading.  Because of this, vertical-motion must examine each and every
> overlay as it moves through the text, because each overlay can
> potentially change the 'invisible' property of text, or it might have
> a display string that needs to be displayed.  So instead of skipping
> all that hidden text in one go, vertical-motion loops over those 17.5K
> overlays examining the properties of each one of them.  And that takes
> time."
>
> I imagine that opening subtree will also require cycling over the
> [many] overlays in the subtree.
>
>> Another option, as I already suggested, would be to use text-properties
>> on property drawers only. Ignoring isearch inside those sounds
>> tolerable, at least.
>
> Hope the patch below is a reasonable solution to isearch problem with
> 'invisible text properties.
>
> Best,
> Ihor
>
> diff --git a/lisp/org-clock.el b/lisp/org-clock.el
> index 34179096d..463b28f47 100644
> --- a/lisp/org-clock.el
> +++ b/lisp/org-clock.el
> @@ -1359,14 +1359,14 @@ the default behavior."
>  	   (sit-for 2)
>  	   (throw 'abort nil))
>  	  (t
> -	   (insert-before-markers "\n")
> +	   (insert-before-markers-and-inherit "\n")
>  	   (backward-char 1)
>  	   (when (and (save-excursion
>  			(end-of-line 0)
>  			(org-in-item-p)))
>  	     (beginning-of-line 1)
>  	     (indent-line-to (- (current-indentation) 2)))
> -	   (insert org-clock-string " ")
> +	   (insert-and-inherit org-clock-string " ")
>  	   (setq org-clock-effort (org-entry-get (point) org-effort-property))
>  	   (setq org-clock-total-time (org-clock-sum-current-item
>  				       (org-clock-get-sum-start)))
> @@ -1658,7 +1658,7 @@ to, overriding the existing value of `org-clock-out-switch-to-state'."
>  	    (if fail-quietly (throw 'exit nil) (error "Clock start time is gone")))
>  	  (goto-char (match-end 0))
>  	  (delete-region (point) (point-at-eol))
> -	  (insert "--")
> +	  (insert-and-inherit "--")
>  	  (setq te (org-insert-time-stamp (or at-time now) 'with-hm 'inactive))
>  	  (setq s (org-time-convert-to-integer
>  		   (time-subtract
> @@ -1666,7 +1666,7 @@ to, overriding the existing value of `org-clock-out-switch-to-state'."
>  		    (org-time-string-to-time ts)))
>  		h (floor s 3600)
>  		m (floor (mod s 3600) 60))
> -	  (insert " => " (format "%2d:%02d" h m))
> +	  (insert-and-inherit " => " (format "%2d:%02d" h m))
>  	  (move-marker org-clock-marker nil)
>  	  (move-marker org-clock-hd-marker nil)
>  	  ;; Possibly remove zero time clocks.  However, do not add
> diff --git a/lisp/org-macs.el b/lisp/org-macs.el
> index a02f713ca..4b0e23f6a 100644
> --- a/lisp/org-macs.el
> +++ b/lisp/org-macs.el
> @@ -682,7 +682,7 @@ When NEXT is non-nil, check the next line instead."
>  
>  
>  \f
> -;;; Overlays
> +;;; Overlays and text properties
>  
>  (defun org-overlay-display (ovl text &optional face evap)
>    "Make overlay OVL display TEXT with face FACE."
> @@ -705,18 +705,44 @@ If DELETE is non-nil, delete all those overlays."
>  	    (delete (delete-overlay ov))
>  	    (t (push ov found))))))
>  
> +(defun org--find-text-property-region (pos prop)
> +  "Find a region containing PROP text property around point POS."
> +  (let* ((beg (and (get-text-property pos prop) pos))
> +	 (end beg))
> +    (when beg
> +      ;; when beg is the first point in the region, `previous-single-property-change'
> +      ;; will return nil.
> +      (setq beg (or (previous-single-property-change pos prop)
> +		    beg))
> +      ;; when end is the last point in the region, `next-single-property-change'
> +      ;; will return nil.
> +      (setq end (or (next-single-property-change pos prop)
> +		    end))
> +      (unless (= beg end) ; this should not happen
> +        (cons beg end)))))
> +
>  (defun org-flag-region (from to flag spec)
>    "Hide or show lines from FROM to TO, according to FLAG.
>  SPEC is the invisibility spec, as a symbol."
> -  (remove-overlays from to 'invisible spec)
> -  ;; Use `front-advance' since text right before to the beginning of
> -  ;; the overlay belongs to the visible line than to the contents.
> -  (when flag
> -    (let ((o (make-overlay from to nil 'front-advance)))
> -      (overlay-put o 'evaporate t)
> -      (overlay-put o 'invisible spec)
> -      (overlay-put o 'isearch-open-invisible #'delete-overlay))))
> -
> +  (pcase spec
> +    ('outline
> +     (remove-overlays from to 'invisible spec)
> +     ;; Use `front-advance' since text right before to the beginning of
> +     ;; the overlay belongs to the visible line than to the contents.
> +     (when flag
> +       (let ((o (make-overlay from to nil 'front-advance)))
> +	 (overlay-put o 'evaporate t)
> +	 (overlay-put o 'invisible spec)
> +	 (overlay-put o 'isearch-open-invisible #'delete-overlay))))
> +    (_
> +     ;; Use text properties instead of overlays for speed.
> +     ;; Overlays are too slow (Emacs Bug#35453).
> +     (with-silent-modifications
> +       (remove-text-properties from to '(invisible nil))
> +       (when flag
> +	 (put-text-property from to 'rear-non-sticky nil)
> +	 (put-text-property from to 'front-sticky t)
> +	 (put-text-property from to 'invisible spec))))))
>  
>  \f
>  ;;; Regexp matching
> diff --git a/lisp/org.el b/lisp/org.el
> index 287fe30e8..335f68a85 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -114,6 +114,7 @@ Stars are put in group 1 and the trimmed body in group 2.")
>  (declare-function cdlatex-math-symbol "ext:cdlatex")
>  (declare-function Info-goto-node "info" (nodename &optional fork strict-case))
>  (declare-function isearch-no-upper-case-p "isearch" (string regexp-flag))
> +(declare-function isearch-filter-visible "isearch" (beg end))
>  (declare-function org-add-archive-files "org-archive" (files))
>  (declare-function org-agenda-entry-get-agenda-timestamp "org-agenda" (pom))
>  (declare-function org-agenda-list "org-agenda" (&optional arg start-day span with-hour))
> @@ -4869,6 +4870,10 @@ The following commands are available:
>    (setq-local outline-isearch-open-invisible-function
>  	      (lambda (&rest _) (org-show-context 'isearch)))
>  
> +  ;; Make isearch search in blocks hidden via text properties
> +  (setq-local isearch-filter-predicate #'org--isearch-filter-predicate)
> +  (add-hook 'isearch-mode-end-hook #'org--clear-isearch-overlays nil 'local)
> +
>    ;; Setup the pcomplete hooks
>    (setq-local pcomplete-command-completion-function #'org-pcomplete-initial)
>    (setq-local pcomplete-command-name-function #'org-command-at-point)
> @@ -5859,9 +5864,26 @@ If TAG is a number, get the corresponding match group."
>  	 (inhibit-modification-hooks t)
>  	 deactivate-mark buffer-file-name buffer-file-truename)
>      (decompose-region beg end)
> +    ;; do not remove invisible text properties specified by
> +    ;; 'org-hide-block and 'org-hide-drawer (but remove  'org-link)
> +    ;; this is needed to keep the drawers and blocks hidden unless
> +    ;; they are toggled by user
> +    ;; Note: The below may be too specific and create troubles
> +    ;; if more invisibility specs are added to org in future
> +    (let ((pos beg)
> +	  next spec)
> +      (while (< pos end)
> +	(setq next (next-single-property-change pos 'invisible nil end)
> +              spec (get-text-property pos 'invisible))
> +        (unless (memq spec (list 'org-hide-block
> +				 'org-hide-drawer))
> +          (remove-text-properties pos next '(invisible t)))
> +        (setq pos next)))
>      (remove-text-properties beg end
>  			    '(mouse-face t keymap t org-linked-text t
> -					 invisible t intangible t
> +					 ;; Do not remove all invisible during fontification
> +					 ;; invisible t
> +                                         intangible t
>  					 org-emphasis t))
>      (org-remove-font-lock-display-properties beg end)))
>  
> @@ -6677,8 +6699,13 @@ information."
>      ;; expose it.
>      (dolist (o (overlays-at (point)))
>        (when (memq (overlay-get o 'invisible)
> -		  '(org-hide-block org-hide-drawer outline))
> +		  '(outline))
>  	(delete-overlay o)))
> +    (when (memq (get-text-property (point) 'invisible)
> +		'(org-hide-block org-hide-drawer))
> +      (let ((spec (get-text-property (point) 'invisible))
> +	    (region (org--find-text-property-region (point) 'invisible)))
> +	(org-flag-region (car region) (cdr region) nil spec)))
>      (unless (org-before-first-heading-p)
>        (org-with-limited-levels
>         (cl-case detail
> @@ -10849,8 +10876,8 @@ EXTRA is additional text that will be inserted into the notes buffer."
>  	 (unless (eq org-log-note-purpose 'clock-out)
>  	   (goto-char (org-log-beginning t)))
>  	 ;; Make sure point is at the beginning of an empty line.
> -	 (cond ((not (bolp)) (let ((inhibit-read-only t)) (insert "\n")))
> -	       ((looking-at "[ \t]*\\S-") (save-excursion (insert "\n"))))
> +	 (cond ((not (bolp)) (let ((inhibit-read-only t)) (insert-and-inherit "\n")))
> +	       ((looking-at "[ \t]*\\S-") (save-excursion (insert-and-inherit "\n"))))
>  	 ;; In an existing list, add a new item at the top level.
>  	 ;; Otherwise, indent line like a regular one.
>  	 (let ((itemp (org-in-item-p)))
> @@ -10860,12 +10887,12 @@ EXTRA is additional text that will be inserted into the notes buffer."
>  				(goto-char itemp) (org-list-struct))))
>  		  (org-list-get-ind (org-list-get-top-point struct) struct)))
>  	     (org-indent-line)))
> -	 (insert (org-list-bullet-string "-") (pop lines))
> +	 (insert-and-inherit (org-list-bullet-string "-") (pop lines))
>  	 (let ((ind (org-list-item-body-column (line-beginning-position))))
>  	   (dolist (line lines)
> -	     (insert "\n")
> +	     (insert-and-inherit "\n")
>  	     (indent-line-to ind)
> -	     (insert line)))
> +	     (insert-and-inherit line)))
>  	 (message "Note stored")
>  	 (org-back-to-heading t))
>  	;; Fix `buffer-undo-list' when `org-store-log-note' is called
> @@ -13036,10 +13063,10 @@ decreases scheduled or deadline date by one day."
>  	      (progn (delete-region (match-beginning 0) (match-end 0))
>  		     (goto-char (match-beginning 0)))
>  	    (goto-char end)
> -	    (insert "\n")
> +	    (insert-and-inherit "\n")
>  	    (backward-char))
> -	  (insert ":" property ":")
> -	  (when value (insert " " value))
> +	  (insert-and-inherit ":" property ":")
> +	  (when value (insert-and-inherit " " value))
>  	  (org-indent-line)))))
>      (run-hook-with-args 'org-property-changed-functions property value)))
>  
> @@ -14177,7 +14204,7 @@ The command returns the inserted time stamp."
>    (let ((fmt (funcall (if with-hm 'cdr 'car) org-time-stamp-formats))
>  	stamp)
>      (when inactive (setq fmt (concat "[" (substring fmt 1 -1) "]")))
> -    (insert-before-markers (or pre ""))
> +    (insert-before-markers-and-inherit (or pre ""))
>      (when (listp extra)
>        (setq extra (car extra))
>        (if (and (stringp extra)
> @@ -14188,8 +14215,8 @@ The command returns the inserted time stamp."
>  	(setq extra nil)))
>      (when extra
>        (setq fmt (concat (substring fmt 0 -1) extra (substring fmt -1))))
> -    (insert-before-markers (setq stamp (format-time-string fmt time)))
> -    (insert-before-markers (or post ""))
> +    (insert-before-markers-and-inherit (setq stamp (format-time-string fmt time)))
> +    (insert-before-markers-and-inherit (or post ""))
>      (setq org-last-inserted-timestamp stamp)))
>  
>  (defun org-toggle-time-stamp-overlays ()
> @@ -20913,6 +20940,79 @@ Started from `gnus-info-find-node'."
>            (t default-org-info-node))))))
>  
>  \f
> +
> +;;; Make isearch search in some text hidden via text propertoes
> +
> +(defvar org--isearch-overlays nil
> +  "List of overlays temporarily created during isearch.
> +This is used to allow searching in regions hidden via text properties.
> +As for [2020-05-09 Sat], Isearch only has special handling of hidden overlays.
> +Any text hidden via text properties is not revealed even if `search-invisible'
> +is set to 't.")
> +
> +;; Not sure if it needs to be a user option
> +;; One might want to reveal hidden text in, for example, hidden parts of the links.
> +;; Currently, hidden text in links is never revealed by isearch.
> +(defvar org-isearch-specs '(org-hide-block
> +			 org-hide-drawer)
> +  "List of text invisibility specs to be searched by isearch.
> +By default ([2020-05-09 Sat]), isearch does not search in hidden text,
> +which was made invisible using text properties. Isearch will be forced
> +to search in hidden text with any of the listed 'invisible property value.")
> +
> +(defun org--create-isearch-overlays (beg end)
> +  "Replace text property invisibility spec by overlays between BEG and END.
> +All the regions with invisibility text property spec from
> +`org-isearch-specs' will be changed to use overlays instead
> +of text properties. The created overlays will be stored in
> +`org--isearch-overlays'."
> +  (let ((pos beg))
> +    (while (< pos end)
> +      (when-let* ((spec (get-text-property pos 'invisible))
> +		  (spec (memq spec org-isearch-specs))
> +		  (region (org--find-text-property-region pos 'invisible)))
> +        ;; Changing text properties is considered buffer modification.
> +        ;; We do not want it here.
> +	(with-silent-modifications
> +          ;; The overlay is modelled after `org-flag-region' [2020-05-09 Sat]
> +          ;; overlay for 'outline blocks.
> +          (let ((o (make-overlay (car region) (cdr region) nil 'front-advance)))
> +	    (overlay-put o 'evaporate t)
> +	    (overlay-put o 'invisible spec)
> +            ;; `delete-overlay' here means that spec information will be lost
> +            ;; for the region. The region will remain visible.
> +	    (overlay-put o 'isearch-open-invisible #'delete-overlay)
> +            (push o org--isearch-overlays))
> +	  (remove-text-properties (car region) (cdr region) '(invisible nil))))
> +      (setq pos (next-single-property-change pos 'invisible nil end)))))
> +
> +(defun org--isearch-filter-predicate (beg end)
> +  "Return non-nil if text between BEG and END is deemed visible by Isearch.
> +This function is intended to be used as `isearch-filter-predicate'.
> +Unlike `isearch-filter-visible', make text with 'invisible text property
> +value listed in `org-isearch-specs' visible to Isearch."
> +  (org--create-isearch-overlays beg end) ;; trick isearch by creating overlays in place of invisible text
> +  (isearch-filter-visible beg end))
> +
> +(defun org--clear-isearch-overlay (ov)
> +  "Convert OV region back into using text properties."
> +  (when-let ((spec (overlay-get ov 'invisible))) ;; ignore deleted overlays
> +    ;; Changing text properties is considered buffer modification.
> +    ;; We do not want it here.
> +    (with-silent-modifications
> +      (put-text-property (overlay-start ov) (overlay-end ov) 'invisible spec)))
> +  (when (member ov isearch-opened-overlays)
> +    (setq isearch-opened-overlays (delete ov isearch-opened-overlays)))
> +  (delete-overlay ov))
> +
> +(defun org--clear-isearch-overlays ()
> +  "Convert overlays from `org--isearch-overlays' back into using text properties."
> +  (when org--isearch-overlays
> +    (mapc #'org--clear-isearch-overlay org--isearch-overlays)
> +    (setq org--isearch-overlays nil)))
> +
> +\f
> +
>  ;;; Finish up
>  
>  (add-hook 'org-mode-hook     ;remove overlays when changing major mode
>
>
>
>
> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>
>> Hello,
>>
>> Ihor Radchenko <yantar92@gmail.com> writes:
>>
>>> ;; Unfortunately isearch, sets inhibit-point-motion-hooks and we
>>> ;; cannot even use cursor-sensor-functions as a workaround
>>> ;; I used a less ideas approach with advice to isearch-search-string as
>>> ;; a workaround 
>>
>> OK.
>>
>>> (defun org-find-text-property-region (pos prop)
>>>   "Find a region containing PROP text property around point POS."
>>>   (require 'org-macs) ;; org-with-point-at
>>>   (org-with-point-at pos
>>
>> Do we really need that since every function has a POS argument anyway?
>> Is it for the `widen' part?
>>
>>>     (let* ((beg (and (get-text-property pos prop) pos))
>>> 	   (end beg))
>>>       (when beg
>>> 	(setq beg (or (previous-single-property-change pos prop)
>>> 		      beg))
>>
>> Shouldn't fall-back be (point-min)?
>>
>>> 	(setq end (or (next-single-property-change pos prop)
>>> 		      end))
>>
>> And (point-max) here?
>>
>>>         (unless (equal beg end)
>>
>> Nitpick: `equal' -> =
>>
>>>           (cons beg end))))))
>>
>>> ;; :FIXME: re-hide properties when point moves away
>>> (define-advice isearch-search-string (:after (&rest _) put-overlay)
>>>   "Reveal hidden text at point."
>>>   (when-let ((region (org-find-text-property-region (point) 'invisible)))
>>>     (with-silent-modifications
>>>       (put-text-property (car region) (cdr region) 'org-invisible (get-text-property (point) 'invisible)))
>>>       (remove-text-properties (car region) (cdr region) '(invisible nil))))
>>
>> Could we use `isearch-update-post-hook' here?
>>
>> Or, it seems nicer to `add-function' around `isearch-filter-predicate'
>> and extend isearch-filter-visible to support (i.e., stop at, and
>> display) invisible text through text properties.
>>
>>> ;; this seems to be unstable, but I cannot figure out why
>>> (defun org-restore-invisibility-specs (&rest _)
>>>   ""
>>>    (let ((pos (point-min)))
>>>      (while (< (setq pos (next-single-property-change pos 'org-invisible nil (point-max))) (point-max))
>>>        (when-let ((region (org-find-text-property-region pos 'org-invisible)))
>>> 	   (with-silent-modifications
>>> 	     (put-text-property (car region) (cdr region) 'invisible (get-text-property pos 'org-invisible))
>>> 	     (remove-text-properties (car region) (cdr region) '(org-invisible nil)))))))
>>
>> Could you use the hook above to store all visited invisible texts, and
>> re-hide them at the end of the search, e.g., using
>> `isearch-mode-end-hook'?
>>
>>> (add-hook 'post-command-hook #'org-restore-invisibility-specs)
>>
>> Ouch. I hope we can avoid that.
>>
>> I wonder how it compares to drawers using the same invisible spec as
>> headlines, as it was the case before. Could you give it a try? 
>>
>> I think hiding all property drawers right after opening a subtree is
>> fast enough.
>>
>> Another option, as I already suggested, would be to use text-properties
>> on property drawers only. Ignoring isearch inside those sounds
>> tolerable, at least.
>>
>> Regards,
>>
>> -- 
>> Nicolas Goaziou
>
> -- 
> Ihor Radchenko,
> PhD,
> Center for Advancing Materials Performance from the Nanoscale (CAMP-nano)
> State Key Laboratory for Mechanical Behavior of Materials, Xi'an Jiaotong University, Xi'an, China
> Email: yantar92@gmail.com, ihor_radchenko@alumni.sutd.edu.sg

-- 
Ihor Radchenko,
PhD,
Center for Advancing Materials Performance from the Nanoscale (CAMP-nano)
State Key Laboratory for Mechanical Behavior of Materials, Xi'an Jiaotong University, Xi'an, China
Email: yantar92@gmail.com, ihor_radchenko@alumni.sutd.edu.sg