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

[Nicolas Goaziou]

Hello,

Ihor Radchenko <address@hidden> writes:

>> As you noticed, using Org Element is a no-go, unfortunately. Parsing an
>> element is a O(N) operation by the number of elements before it in
>> a section. In particular, it is not bounded, and not mitigated by
>> a cache. For large documents, it is going to be unbearably slow, too.
>
> Ouch. I thought it is faster.
> What do you mean by "not mitigated by a cache"?

Parsing starts from the closest headline, every time. So, if Org parses
the Nth element in the entry two times, it really parses 2N elements.

With a cache, assuming the buffer wasn't modified, Org would parse
N elements only. With a smarter cache, with fine grained cache
invalidation, it could also reduce the number of subsequent parsed
elements.

> The reason I would like to utilise org-element parser to make tracking
> modifications more robust. Using details of the syntax would make the
> code fragile if any modifications are made to syntax in future.

I don't think the code would be more fragile. Also, the syntax we're
talking about is not going to be modified anytime soon. Moreover, if
folding breaks, it is usually visible, so the bug will not be unnoticed.

This code is going to be as low-level as it can be.

> Debugging bugs in modification functions is not easy, according to my
> experience.

No, it's not. 

But this is not really related to whether you use Element or not.

> One possible way to avoid performance issues during modification is
> running parser in advance. For example, folding an element may
> as well add information about the element to its text properties.
> This will not degrade performance of folding since we are already
> parsing the element during folding (at least, in
> org-hide-drawer-toggle).

We can use this information stored at fold time. But I'm not even sure
we need it.

> The problem with parsing an element during folding is that we cannot
> easily detect changes like below without re-parsing.

Of course we can. It is only necessary to focus on changes that would
break the structure of the element. This does not entail a full parsing.

> :PROPERTIES: <folded>
> :CREATED: [2020-05-18 Mon]
> :END: <- added line
> :ID: test
> :END:
>
> or even
>
> :PROPERTIES:
> :CREATED: [2020-05-18 Mon]
> :ID: test
> :END: <- delete this line
>
> :DRAWER: <folded, cannot be unfolded if we don't re-parse after deletion>
> test
> :END:

Please have a look at the "sensitive parts" I wrote about. This takes
care of this kind of breakage.

> The re-parsing can be done via regexp, as you suggested, but I don't
> like this idea, because it will end up re-implementing
> org-element-*-parser.

You may have misunderstood my suggestion. See below.

> Would it be acceptable to run org-element-*-parser
> in after-change-functions?

I'd rather not do that. This is unnecessary consing, and matching, etc.

> If I understand correctly, it is not as easy.
> Consider the following example:
>
> :PROPERTIES:
> :CREATED: [2020-05-18 Mon]
> <region-beginning>
> :ID: example
> :END:
>
> <... a lot of text, maybe containing other drawers ...>
>
> Nullam rutrum.
> Pellentesque dapibus suscipit ligula.
> <region-end>
> Proin quam nisl, tincidunt et, mattis eget, convallis nec, purus.
>
> If the region gets deleted, the modification hooks from chars inside
> drawer will be called as (hook-function <region-beginning>
> <region-end>). So, there is still a need to find the drawer somehow to
> mark it as about to be modified (modification hooks are ran before
> actual modification).

If we can stick with `after-change-functions' (or local equivalent),
that's better. It is more predictable than `before-change-functions' and
alike.

If it is a deletion, here is the kind of checks we could do, depending
on when they are performed.

Before actual changes :

  1. The deletion is happening within a folded drawer (unnecessary step
     in local functions).
  2. The change deleted the sensitive line ":END:".
  3. Conclusion : unfold.

Or, after actual changes :

  1. The deletion involves a drawer.
  2. Text properties indicate that the beginning of the propertized part
     of the buffer start with org-drawer-regexp, but doesn't end with
     `org-property-end-re'. A "sensitive part" disappeared!
  3. Conclusion : unfold

This is far away from parsing. IMO, a few checks cover all cases. Let me
know if you have questions about it.

Also, note that the kind of change you describe will happen perhaps
0.01% of the time. Most change are about one character, or a single
line, long.

> The only difference between using modification hooks and
> before-change-functions is that modification hooks will trigger less
> frequently. 

Exactly. Much less frequently. But extra care is required, as you noted
already.

> Considering the performance of org-element-at-point, it is
> probably worth doing. Initially, I wanted to avoid it because setting a
> single before-change-functions hook sounded cleaner than setting
> modification-hooks, insert-behind-hooks, and insert-in-front-hooks.

Well, `before-change-fuctions' and `after-change-functions' are not
clean at all: you modify an unrelated part of the buffer, but still call
those to check if a drawer needs to be unfolded somewhere.

And, more importantly, they are not meant to be used together, i.e., you
cannot assume that a single call to `before-change-functions' always
happens before calling `after-change-functions'. This can be tricky if
you want to use the former to pass information to the latter.

But I understand that they are easier to use than their local
counterparts. If you stick with (before|after)-change-functions, the
function being called needs to drop the ball very quickly if the
modification is not about folding changes. Also, I very much suggest to
stick to only `after-change-functions', if feasible (I think it is), per
above.

> Moreover, these text properties would be copied by default if one uses 
> buffer-substring. Then, the hooks will also trigger later in the yanked
> text, which may cause all kinds of bugs.

Indeed, that would be something to handle specifically. I.e.,
destructive modifications (i.e., those that unfold) could clear such
properties.

It is more work. I don't know if it is worth the trouble if we can get
out quickly of `after-change-functions' for unrelated changes.

> It was mostly an annoyance, because they returned different results on
> the same element. Specifically, they returned different :post-blank and
> :end properties, which does not sound right.

OK. If you have a reproducible recipe, I can look into it and see what
can be done.

Regards,

-- 
Nicolas Goaziou