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

[Ihor Radchenko]

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."
 
 
 
-;;; 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))))))
 
 
 ;;; 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))))))
 
 
+
+;;; 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)))
+
+
+
 ;;; Finish up
 
 (add-hook 'org-mode-hook     ;remove overlays when changing major mode




Nicolas Goaziou <address@hidden> writes:

> Hello,
>
> Ihor Radchenko <address@hidden> 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: address@hidden, address@hidden