JV <[email protected]> writes: >> Try opening the attached file and scrolling. >> The performance is severely degraded with your patch. > > I compared performance with many indented list items to other repeated > fontified elements. The previous patch version takes 2.5x as long in > font-lock-fontify-region for indented list items as headlines. I've been > able to get this down to 1.5x. > > The most expensive calls are ones to the org-element API. > org-element-at-point and org-element-lineage are the main contributors. > In a tight loop, they add up. Even replacing org-element-type with 'car > yields a measurable improvement. And GC is definitely a factor.
Not unexpected. This is one of the reasons holding me off from working on rewriting all the fontification to use org-element API, even though we will need to do it sooner or later. > If you have a particular performance standard you think this should > meet, please let me know. Otherwise I will squeeze what's available, > benchmark it, and if it feels responsive in the extreme test case, send > it for your review. If we are talking about 10-20%, I do not see a bit problem, especially if the feature is made optional, and disabled by default. Otherwise, redisplay is one of the sensitive things in Emacs - if it is slow, it degrades user experience a lot. >> We should not remove existing groups. >> Not because it is a good thing to have them, but because third-party code >> may rely on the current behavior. > > One can't depend on match groups in the current version: group numbering > differs based on which way the pattern matches. This is the reason we're > patching this function. Fair. But then we also need to document the changes in ORG-NEWS. >> Also, :package-version should be added to defface I think. > > What package version should I list? 10.0 - next release. > >>> (defface org-hide > >>> - '((((background light)) (:foreground "white")) > >>> + '((default :inherit org-structure) > >>> + (((background light)) (:foreground "white")) > >>> (((background dark)) (:foreground "black"))) > > >> I think we also need (t) to apply default there. > > This appears ok to me, and works as expected. Have I missed something? Hmm. No. I confused things. -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
