From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id 0MBsFvzI3V5qQwAA0tVLHw (envelope-from ) for ; Mon, 08 Jun 2020 05:13:32 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1 with LMTPS id GGEVEvzI3V6ZLgAAbx9fmQ (envelope-from ) for ; Mon, 08 Jun 2020 05:13:32 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id BAA769400B1 for ; Mon, 8 Jun 2020 05:13:31 +0000 (UTC) Received: from localhost ([::1]:34280 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jiA6E-0007B5-Om for larch@yhetil.org; Mon, 08 Jun 2020 01:13:30 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:55738) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jiA5u-0007Ax-Nm for emacs-orgmode@gnu.org; Mon, 08 Jun 2020 01:13:10 -0400 Received: from mail-pl1-x62e.google.com ([2607:f8b0:4864:20::62e]:36134) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jiA5t-0003Fz-AN for emacs-orgmode@gnu.org; Mon, 08 Jun 2020 01:13:10 -0400 Received: by mail-pl1-x62e.google.com with SMTP id bg4so6196968plb.3 for ; Sun, 07 Jun 2020 22:13:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:in-reply-to:references:date:message-id :mime-version; bh=oOWkenUEeM9Kno1bS1SkoZ8VZPSjHOtartmLa4d3xes=; b=j646KyxFakg3Fg0GztsMkvtA8oGgi7J5/nSDETxWKRYyoTzzY7YD66TE+vZKIyGjt1 7pXrIsoNDAeylRxSLz1YljWV6ZfCY53Vl34ydx+Xr1N2FflgCASFqBRIcaroSxD4z3Va iX/YcVxS/yx+czN+UoEH4JWjd17KFunRIi356h/uC+uw2yqb+E7A8qVFD92Y6AEAR+Pm YAoCEv2EJuDC6fGw4HJKRnu8C6ynnsM/4KcQ344/ojkxSQ/BuGvA1ZGOx+mnJWwDqP8U xpD+6YvKyduAJF0CTsAShUlb/qRqXBo8PwtpyukyF9ejRcfRz0d5M6IM4ZPmO3hSX9QD QArQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=oOWkenUEeM9Kno1bS1SkoZ8VZPSjHOtartmLa4d3xes=; b=DasWSPu9l/JWSdiWNuOHdU2ueCuC6IFGBhQ8Lj1CuytDboTd+jFv0YL/lFrbMggD5A O4ROggKurygJgZnDb4f+stgXHVQ7DnoNwXg0IetnMXhc/Bvu3LPakJ1DRUa3sy4Un2XF eR656ZpcXuJdML85H829WdzXdcBBW+OxrnAyDwhm3SE/koDq4YxGDecjkHC88DvNU+yz Q6R+sgfoJcBp4Cu54I2Jb9vYmtbqmw0k8zuxgpwfXFL+o8seSteKBcqdKvQrDEA/RTON JRnBPPIL9as3wgNZ6b7zsmSCLT/T6hZAWuvD+q7QwuDDECi7aGeBalA7KvrDsTnGbacF F3Fg== X-Gm-Message-State: AOAM532G1PdPZUPnIoO63GMJJZXFgTFCOy/ebCQZfvcNor2/dsIhE72f 7udUJr1n9ODxUjfOHd3yaM+5rn+1HBM= X-Google-Smtp-Source: ABdhPJzt4Q61o7CGf0AG9mQUIATpuQiuUZ6AEEM9umAy6IvlhxWyDmXxQoZ4kxZvG+NPzn7K/+h1MQ== X-Received: by 2002:a17:90b:3691:: with SMTP id mj17mr13798800pjb.152.1591593187714; Sun, 07 Jun 2020 22:13:07 -0700 (PDT) Received: from localhost ([210.3.160.230]) by smtp.gmail.com with ESMTPSA id i22sm5799065pfo.92.2020.06.07.22.13.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 07 Jun 2020 22:13:07 -0700 (PDT) From: Ihor Radchenko To: Nicolas Goaziou Subject: Re: [patch suggestion] Mitigating the poor Emacs performance on huge org files: Do not use overlays for PROPERTY and LOGBOOK drawers In-Reply-To: <875zc2du63.fsf@localhost> References: <87h7x9e5jo.fsf@localhost> <87r1vu5qmc.fsf@nicolasgoaziou.fr> <87imh5w1zt.fsf@localhost> <87blmxjckl.fsf@localhost> <87y2q13tgs.fsf@nicolasgoaziou.fr> <878si1j83x.fsf@localhost> <87d07bzvhd.fsf@nicolasgoaziou.fr> <87imh34usq.fsf@localhost> <87pnbby49m.fsf@nicolasgoaziou.fr> <87tv0efvyd.fsf@localhost> <874kse1seu.fsf@localhost> <87r1vhqpja.fsf@nicolasgoaziou.fr> <87tv0d2nk7.fsf@localhost> <87o8qkhy3g.fsf@nicolasgoaziou.fr> <87sgfqu5av.fsf@localhost> <87sgfn6qpc.fsf@nicolasgoaziou.fr> <87367d4ydc.fsf@localhost> <87r1uuotw8.fsf@nicolasgoaziou.fr> <87mu5iq618.fsf@localhost> <87ftb9pqop.fsf@nicolasgoaziou.fr> <875zc2du63.fsf@localhost> Date: Mon, 08 Jun 2020 13:08:19 +0800 Message-ID: <87zh9ecfgc.fsf@localhost> MIME-Version: 1.0 Content-Type: text/plain Received-SPF: pass client-ip=2607:f8b0:4864:20::62e; envelope-from=yantar92@gmail.com; helo=mail-pl1-x62e.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -17 X-Spam_score: -1.8 X-Spam_bar: - X-Spam_report: (-1.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_ENVFROM_END_DIGIT=0.25, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: emacs-orgmode@gnu.org Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=j646KyxF; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (aspmx1.migadu.com: domain of emacs-orgmode-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=emacs-orgmode-bounces@gnu.org X-Spam-Score: -1.21 X-TUID: PUVuI0smuk96 Github link to the patch: https://gist.github.com/yantar92/6447754415457927293acda43a7fcaef Ihor Radchenko writes: > Hello, > > [The patch itself will be provided in the following email] > > I have four more updates from the previous version of the patch: > > 1. All the code handling modifications in folded drawers/blocks is moved > to after-change-function. It works as follows: > - if any text is inserted in the middle of hidden region, that text > is also hidden; > - if BEGIN/END line of a folded drawer do not match org-drawer-regexp > and org-property-end-re, unfold it; > - if org-property-end-re or new org-outline-regexp-bol is inserted in > the middle of the drawer, unfold it; > - the same logic for blocks. > > 2. The text property stack is rewritten using char-property-alias-alist. > This is faster in comparison with previous approach, which involved > modifying all the text properties every timer org-flag-region was > called. > > 3. org-toggle-custom-properties-visibility is rewritten using text > properties. I also took a freedom to implement a new feature here. > Now, setting new `org-custom-properties-hide-emptied-drawers' to > non-nil will result in hiding the whole property drawer if it > contains only org-custom-properties. > > 4. This patch should work against 1aa095ccf. However, the merge was not > trivial here. Recent commits actively used the fact that drawers and > outlines are hidden via 'outline invisibility spec, which is not the > case in this branch. I am not confident that I did not break anything > during the merge, especially 1aa095ccf. > > ----------------------------------------------------------------------- > ----------------------------------------------------------------------- > > More details on the new implementation for tracking changes: > >> I gave you a few ideas to quickly check if a change requires expansion, >> in an earlier mail. I suggest to start out from that. Let me know if you >> have questions about it. > > All the code lives in org-after-change-function. I tried to incorporate > the earlier Nicholas' suggestions, except the parts related to > intersecting blocks and drawers. I am not sure if I understand the > parsing priority of blocks vs. drawers. > > ----------------------------------------------------------------------- > ----------------------------------------------------------------------- > > More details on the text property stack: > > The earlier version of the code literally used stack to save > pre-existing 'invisibility specs in org-flag-region. This was done on > every invocation of org-flag-region, which made org-flag-region > significantly slower. I re-implemented the same feature using > char-property-alias-alist. Now, different invisibility specs live in > separate text properties and can be safely modified independently. The > specs are applied according to org--invisible-spec-priority-list. A side > effect of current implementation is that char-property-alias-alist is > fully controlled by org. All the pre-existing settings for 'invisible > text property will be overwritten by org. > >> `gensym' is just a shorter, and somewhat standard way, to create a new >> uninterned symbol with a given prefix. You seem to re-invent it. What >> you do with that new symbol is orthogonal to that suggestion, of course. > > I do not think that `gensym' is suitable here. We don't want a new > symbol every time org--get-buffer-local-invisible-property-symbol is > called. It should return the same symbol if it is called from the same > buffer multiple times. > > ----------------------------------------------------------------------- > ----------------------------------------------------------------------- > > More details on the org-toggle-custom-properties-visibility: > > The implementation showcases how to introduce new invisibility specs to > org. Apart from expected (add-to-invisibility-spec 'org-hide-custom-property) > one also needs to add the spec into org--invisible-spec-priority-list: > > (add-to-list 'org--invisible-spec-priority-list 'org-hide-custom-property) > > Searching for text with the given invisibility spec is done as > follows: > > (text-property-search-forward (org--get-buffer-local-invisible-property-symbol 'org-hide-custom-property) 'org-hide-custom-property t) > > This last piece of code is probably not the most elegant. I am thinking > if creating some higher-level interface would be more reasonable here. > What do you think? > > > The new customisation `org-custom-properties-hide-emptied-drawers' > sounds logical for me since empty property drawers left after invoking > org-toggle-custom-properties-visibility are rather useless according to > my experience. If one already wants to hide parts of property drawers, I > do not see a reason to show leftover > > :PROPERTIES: > :END: > > ----------------------------------------------------------------------- > ----------------------------------------------------------------------- > > More details on the merge with the latest master: > > I tried my best to not break anything. However, I am not sure if I > understand all the recent commits. Could someone take a look if there is > anything suspicious in org-next-visible-heading? > > Also, I have seen some optimisations making use of the fact that drawers > and headlines both use 'outline invisibility spec. This change in the > implementation details supposed to improve performance and should not be > necessary if this patch is going to be merged. Would it be possible to > refrain from abusing this particular implementation detail in the > nearest commits on master (unless really necessary)? > > ----------------------------------------------------------------------- > ----------------------------------------------------------------------- > > Further work: > > I would like to finalise the current patch and work on other code using > overlays separately. This patch is already quite complicated as is. I do > not want to introduce even more potential bugs by working on things not > directly affected by this version of the patch. > > Best, > Ihor > > > Nicolas Goaziou writes: > >> Ihor Radchenko writes: >> >>>> See also `gensym'. Do we really need to use it for something else than >>>> `invisible'? If not, the tool doesn't need to be generic. >>> >>> For now, I also use it for buffer-local 'invisible stack. The stack is >>> needed to preserve folding state of drawers/blocks inside folded >>> outline. Though I am thinking about replacing the stack with separate >>> text properties, like 'invisible-outline-buffer-local + >>> 'invisible-drawer-buffer-local + 'invisible-block-buffer-local. >>> Maintaining stack takes a noticeable percentage of CPU time in profiler. >>> >>> org--get-buffer-local-text-property-symbol must take care about >>> situation with indirect buffers. When an indirect buffer is created from >>> some org buffer, the old value of char-property-alias-alist is carried >>> over. We need to detect this case and create new buffer-local symbol, >>> which is unique to the newly created buffer (but not create it if the >>> buffer-local property is already there). Then, the new symbol must >>> replace the old alias in char-property-alias-alist + old folding state >>> must be preserved (via copying the old invisibility specs into the new >>> buffer-local text property). I do not see how gensym can benefit this >>> logic. >> >> `gensym' is just a shorter, and somewhat standard way, to create a new >> uninterned symbol with a given prefix. You seem to re-invent it. What >> you do with that new symbol is orthogonal to that suggestion, of course. >> >>>> OK, but this may not be sufficient if we want to do slightly better than >>>> overlays in that area. This is not mandatory, though. >>> >>> Could you elaborate on what can be "slightly better"? >> >> IIRC, I gave examples of finer control of folding state after a change. >> Consider this _folded_ drawer: >> >> :BEGIN: >> Foo >> :END: >> >> Inserting ":END" in it should not unfold it, as it is currently the case >> with overlays, >> >> :BEGIN >> Foo >> :END >> :END: >> >> but a soon as the last ":" is inserted, the initial drawer could be >> expanded. >> >> :BEGIN >> Foo >> :END: >> :END: >> >> The latter case is not currently handled by overlays. This is what >> I call "slightly better". >> >> Also, note that this change is not related to opening and closing lines >> of the initial drawer, so sticking text properties on them would not >> help here. >> >> Another case is modifying those borders, e.g., >> >> >> :BEGIN: :BEGIN: >> Foo ------> Foo >> :END: :ND: >> >> which should expand the drawer. Your implementation catches this, but >> I'm pointing out that current implementation with overlays does not. >> Even though that's not strictly required for compatibility with >> overlays, it is a welcome slight improvement. >> >>>> As discussed before, I don't think you need to use `modification-hooks' >>>> or `insert-behind-hooks' if you already use `after-change-functions'. >>>> >>>> `after-change-functions' are also triggered upon text properties >>>> changes. So, what is the use case for the other hooks? >>> >>> The problem is that `after-change-functions' cannot be a text property. >>> Only `modification-hooks' and `insert-in-front/behind-hooks' can be a >>> valid text property. If we use `after-change-functions', they will >>> always be triggered, regardless if the change was made inside or outside >>> folded region. >> >> As discussed, text properties are local to the change, but require extra >> care when moving text around. You also observed serious overhead when >> using them. >> >> OTOH, even if `a-c-f' is not local, you can quickly determine if the >> change altered a folded element, so the overhead is limited, i.e., >> mostly checking for a text property at a given buffer position. >> >> To be clear, I initially thought that text properties were a superior >> choice, but I changed my mind a while ago, and I thought you had, too. >> IOW, `after-change-functions' is the way to go, since you have no strong >> reason to stick to text properties for this kind of function. >> >>>>> :asd: >>>>> :drawer: >>>>> lksjdfksdfjl >>>>> sdfsdfsdf >>>>> :end: >>>>> >>>>> If :asd: was inserted in front of folded :drawer:, changes in :drawer: >>>>> line of the new folded :asd: drawer would reveal the text between >>>>> :drawer: and :end:. >>>>> >>>>> Let me know what you think on this. >>> >>>> I have first to understand the use case for `modification-hook'. But >>>> I think unfolding is the right thing to do in this situation, isn't it? >>> >>> That situation arises because the modification-hooks from ":drawer:" >>> (they are set via text properties) only have information about the >>> :drawer:...:end: drawer before the modifications (they were set when >>> :drawer: was folded last time). So, they will only unfold a part of the >>> new :asd: drawer. I do not see a simple way to unfold everything without >>> re-parsing the drawer around the changed text. >> >> Oh! I misread your message. I withdraw what I wrote. In this case, we >> don't want to unfold anything. The situation is not worse than what we >> have now, and trying to fix it would have repercussions down in the >> buffer, e.g., expanding drawers screen below. >> >> As a rule of thumb, I think we can pay attention to changes in the >> folded text, and its immediate surroundings (e.g., the opening line, >> which is not folded), but no further. >> >> As written above, slight changes are welcome, but let's not go overboard >> and parse a whole section just to know if we can expand a drawer. >> >>> Actually, I am quite unhappy with the performance of modification-hooks >>> set via text properties (I am using this patch on my Emacs during this >>> week). It appears that setting the text properties costs a significant >>> CPU time in practice, even though running the hooks is pretty fast. >>> I will think about a way to handle modifications using global >>> after-change-functions. >> >> That's better, IMO. >> >> I gave you a few ideas to quickly check if a change requires expansion, >> in an earlier mail. I suggest to start out from that. Let me know if you >> have questions about it. > > -- > 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