Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
Ilya Shlyakhter ilya_...@alum.mit.edu wrote: A corrected patch is attached. One more thing that you'll need to do is put your patches in attachments of a type that will allow patchwork to snag the patch: application/octet-stream does not. And if they don't end up in patchwork, they might get lost. See the Patches get caught on patchwork section at http://orgmode.org/worg/org-contribute.html#sec-4-3 Nick
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
Hi Ilya, hi Nick, thanks for looking into this. I am amazed by the deep understanding of Org's internals that shows in this thread. Both patches seem to be OK as far as I can see and can be applied without adverse effects. The patch for org-clock.el will at most achieve a factor of two (because org-get-tags-at is called anyway), but indeed, the patch in org.el can potentially have even more significant effects, when properties are tested in the matcher. Cheers and thanks! - Carsten On 16.3.2012, at 05:34, Ilya Shlyakhter wrote: Here is a similar patch for org-clock's use of tags/properties matcher. On Fri, Mar 16, 2012 at 12:31 AM, Ilya Shlyakhter ilya_...@alum.mit.edu wrote: , | If your function needs to retrieve the tags including inherited tags | at the *current* entry, 'Function' here refers to the FUNC parameter of org-map-entries, not the MATCHER parameter. The matcher is constructed by org-make-tags-matcher, so we know everything it does -- it does not move around and only asks about the current entry's tags and properties. org-scan-tags only invokes the matcher at the current entry, and sets org-scanner-tags correctly for that call. But, you're right that there is a problem: while org-scan-tags sets org-scanner-tags correctly before (eval matcher), other users of the matcher -- e.g. org-clock-get-table-data -- might not. So, org-trust-scanner-tags should be set not in the matcher, but in the function that calls the matcher. A corrected patch is attached. thanks, ilya On Thu, Mar 15, 2012 at 11:13 PM, Nick Dokos nicholas.do...@hp.com wrote: Ilya Shlyakhter ilya_...@alum.mit.edu wrote: The attached patch speeds up tags matching ( 50s -- 5s for my most common search ), by turning on org-trust-scanner-tags within the matcher. (When it's off, getting a non-inherited property's value causes a call to org-entry-properties to fetch all properties into a cache, including ALLTAGS; fetching ALLTAGS involves calling (org-get-tags-at), which is slow when org-trust-scanner-tags is off.) Can this cause problems / was this off for a reason? I haven't looked at your patch carefully enough to know if it will or will not cause problems, but check the doc for org-map-entries: it has some guidelines about where the technique can be used and where it cannot: , | If your function needs to retrieve the tags including inherited tags | at the *current* entry, you can use the value of the variable | `org-scanner-tags' which will be much faster than getting the value | with `org-get-tags-at'. If your function gets properties with | `org-entry-properties' at the *current* entry, bind `org-trust-scanner-tags' | to t around the call to `org-entry-properties' to get the same speedup. | Note that if your function moves around to retrieve tags and properties at | a *different* entry, you cannot use these techniques. ` There are warnings that this variable is for internal dynamical scoping only, so I suspect you should not mess with the default. If your search can make the needed guarantees, then you can just wrap it in a let to get the speedup. Otherwise, it probably should be left alone. Nick 0003-Clocking-work-time-faster-filtering-of-clock-entries.patch
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
On 3/16/2012 2:10 AM, Nick Dokos wrote: One more thing that you'll need to do is put your patches in attachments of a type that will allow patchwork to snag the patch: Thanks, I was wondering why they're not showing up. Here is another try (attached) for the org.el patch. ilya From 95c38b06803aec0787bc2eaab3d0062221390292 Mon Sep 17 00:00:00 2001 From: Ilya Shlyakhter ilya_...@alum.mit.edu Date: Fri, 16 Mar 2012 00:10:25 -0400 Subject: [PATCH 2/2] Tags/properties matcher: faster matching by trusting org-scanner-tags * lisp/org.el (org-scan-tags): Bind org-trust-scanner-tags to t while evaluating the matcher, since the matcher is always evaluated at the current entry. TINYCHANGE --- lisp/org.el |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lisp/org.el b/lisp/org.el index ad63213..951f692 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -12906,7 +12906,8 @@ headlines matching this string. ;; eval matcher only when the todo condition is OK (and (or (not todo-only) (member todo org-not-done-keywords)) - (let ((case-fold-search t)) (eval matcher))) + (let ((case-fold-search t) (org-trust-scanner-tags t)) + (eval matcher))) ;; Call the skipper, but return t if it does not skip, ;; so that the `and' form continues evaluating -- 1.7.9.3
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
On 3/16/2012 2:10 AM, Nick Dokos wrote: One more thing that you'll need to do is put your patches in attachments of a type that will allow patchwork to snag the patch: And here is the org-clock.el patch again. From 4f7f91ae62d425f7a89738b28006b1743a6bea4d Mon Sep 17 00:00:00 2001 From: Ilya Shlyakhter ilya_...@alum.mit.edu Date: Fri, 16 Mar 2012 00:25:18 -0400 Subject: [PATCH 3/3] Clocking work time: faster filtering of clock entries by trusting org-scanner-tags * lisp/org-clock.el (org-clock-get-table-data): Bind org-scanner-tags to tags-list and org-trust-scanner-tags to t while evaluating the matcher, since the matcher is always evaluated at the current entry. TINYCHANGE --- lisp/org-clock.el |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lisp/org-clock.el b/lisp/org-clock.el index 9206608..46d9af8 100644 --- a/lisp/org-clock.el +++ b/lisp/org-clock.el @@ -2463,7 +2463,9 @@ TIME: The sum of all time spend in this tree, in minutes. This time (org-clock-sum ts te (unless (null matcher) (lambda () -(let ((tags-list (org-get-tags-at))) +(let* ((tags-list (org-get-tags-at)) + (org-scanner-tags tags-list) + (org-trust-scanner-tags t)) (eval matcher) (goto-char (point-min)) (setq st t) -- 1.7.9.3
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
Carsten Dominik carsten.domi...@gmail.com wrote: Hi Ilya, hi Nick, thanks for looking into this. I am amazed by the deep understanding of Org's internals that shows in this thread. On Ilya's part, certainly - for my part, I have only the vaguest clue about what Ilya did - I just saw a possible red flag. I still have not looked into his patches, but now, thanks to you, I don't have to :-) I can just enjoy the speedup! Nick Both patches seem to be OK as far as I can see and can be applied without adverse effects. The patch for org-clock.el will at most achieve a factor of two (because org-get-tags-at is called anyway), but indeed, the patch in org.el can potentially have even more significant effects, when properties are tested in the matcher. Cheers and thanks! - Carsten
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
Applied, thanks! - Carsten On Mar 16, 2012, at 3:13 PM, Ilya Shlyakhter wrote: On 3/16/2012 2:10 AM, Nick Dokos wrote: One more thing that you'll need to do is put your patches in attachments of a type that will allow patchwork to snag the patch: Thanks, I was wondering why they're not showing up. Here is another try (attached) for the org.el patch. ilya 0002-Tags-properties-matcher-faster-matching-by-trusting-.patch - Carsten
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
This one again did not make it to patchwork, but I have applied it anyway, thanks. - Carsten On Mar 16, 2012, at 3:20 PM, Ilya Shlyakhter wrote: On 3/16/2012 2:10 AM, Nick Dokos wrote: One more thing that you'll need to do is put your patches in attachments of a type that will allow patchwork to snag the patch: And here is the org-clock.el patch again. 0003-Clocking-work-time-faster-filtering-of-clock-entries.patch - Carsten
[O] [PATCH] tags search: faster tags matcher by trusting scanner tags
The attached patch speeds up tags matching ( 50s -- 5s for my most common search ), by turning on org-trust-scanner-tags within the matcher. (When it's off, getting a non-inherited property's value causes a call to org-entry-properties to fetch all properties into a cache, including ALLTAGS; fetching ALLTAGS involves calling (org-get-tags-at), which is slow when org-trust-scanner-tags is off.) Can this cause problems / was this off for a reason? thanks, ilya 0022-Tags-matcher-turned-on-org-trust-scanner-tags-within.patch Description: Binary data
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
Ilya Shlyakhter ilya_...@alum.mit.edu wrote: The attached patch speeds up tags matching ( 50s -- 5s for my most common search ), by turning on org-trust-scanner-tags within the matcher. (When it's off, getting a non-inherited property's value causes a call to org-entry-properties to fetch all properties into a cache, including ALLTAGS; fetching ALLTAGS involves calling (org-get-tags-at), which is slow when org-trust-scanner-tags is off.) Can this cause problems / was this off for a reason? I haven't looked at your patch carefully enough to know if it will or will not cause problems, but check the doc for org-map-entries: it has some guidelines about where the technique can be used and where it cannot: , | If your function needs to retrieve the tags including inherited tags | at the *current* entry, you can use the value of the variable | `org-scanner-tags' which will be much faster than getting the value | with `org-get-tags-at'. If your function gets properties with | `org-entry-properties' at the *current* entry, bind `org-trust-scanner-tags' | to t around the call to `org-entry-properties' to get the same speedup. | Note that if your function moves around to retrieve tags and properties at | a *different* entry, you cannot use these techniques. ` There are warnings that this variable is for internal dynamical scoping only, so I suspect you should not mess with the default. If your search can make the needed guarantees, then you can just wrap it in a let to get the speedup. Otherwise, it probably should be left alone. Nick
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
, | If your function needs to retrieve the tags including inherited tags | at the *current* entry, 'Function' here refers to the FUNC parameter of org-map-entries, not the MATCHER parameter. The matcher is constructed by org-make-tags-matcher, so we know everything it does -- it does not move around and only asks about the current entry's tags and properties. org-scan-tags only invokes the matcher at the current entry, and sets org-scanner-tags correctly for that call. But, you're right that there is a problem: while org-scan-tags sets org-scanner-tags correctly before (eval matcher), other users of the matcher -- e.g. org-clock-get-table-data -- might not. So, org-trust-scanner-tags should be set not in the matcher, but in the function that calls the matcher. A corrected patch is attached. thanks, ilya On Thu, Mar 15, 2012 at 11:13 PM, Nick Dokos nicholas.do...@hp.com wrote: Ilya Shlyakhter ilya_...@alum.mit.edu wrote: The attached patch speeds up tags matching ( 50s -- 5s for my most common search ), by turning on org-trust-scanner-tags within the matcher. (When it's off, getting a non-inherited property's value causes a call to org-entry-properties to fetch all properties into a cache, including ALLTAGS; fetching ALLTAGS involves calling (org-get-tags-at), which is slow when org-trust-scanner-tags is off.) Can this cause problems / was this off for a reason? I haven't looked at your patch carefully enough to know if it will or will not cause problems, but check the doc for org-map-entries: it has some guidelines about where the technique can be used and where it cannot: , | If your function needs to retrieve the tags including inherited tags | at the *current* entry, you can use the value of the variable | `org-scanner-tags' which will be much faster than getting the value | with `org-get-tags-at'. If your function gets properties with | `org-entry-properties' at the *current* entry, bind `org-trust-scanner-tags' | to t around the call to `org-entry-properties' to get the same speedup. | Note that if your function moves around to retrieve tags and properties at | a *different* entry, you cannot use these techniques. ` There are warnings that this variable is for internal dynamical scoping only, so I suspect you should not mess with the default. If your search can make the needed guarantees, then you can just wrap it in a let to get the speedup. Otherwise, it probably should be left alone. Nick 0002-Tags-properties-matcher-faster-matching-by-trusting-.patch Description: Binary data
Re: [O] [PATCH] tags search: faster tags matcher by trusting scanner tags
Here is a similar patch for org-clock's use of tags/properties matcher. On Fri, Mar 16, 2012 at 12:31 AM, Ilya Shlyakhter ilya_...@alum.mit.eduwrote: , | If your function needs to retrieve the tags including inherited tags | at the *current* entry, 'Function' here refers to the FUNC parameter of org-map-entries, not the MATCHER parameter. The matcher is constructed by org-make-tags-matcher, so we know everything it does -- it does not move around and only asks about the current entry's tags and properties. org-scan-tags only invokes the matcher at the current entry, and sets org-scanner-tags correctly for that call. But, you're right that there is a problem: while org-scan-tags sets org-scanner-tags correctly before (eval matcher), other users of the matcher -- e.g. org-clock-get-table-data -- might not. So, org-trust-scanner-tags should be set not in the matcher, but in the function that calls the matcher. A corrected patch is attached. thanks, ilya On Thu, Mar 15, 2012 at 11:13 PM, Nick Dokos nicholas.do...@hp.comwrote: Ilya Shlyakhter ilya_...@alum.mit.edu wrote: The attached patch speeds up tags matching ( 50s -- 5s for my most common search ), by turning on org-trust-scanner-tags within the matcher. (When it's off, getting a non-inherited property's value causes a call to org-entry-properties to fetch all properties into a cache, including ALLTAGS; fetching ALLTAGS involves calling (org-get-tags-at), which is slow when org-trust-scanner-tags is off.) Can this cause problems / was this off for a reason? I haven't looked at your patch carefully enough to know if it will or will not cause problems, but check the doc for org-map-entries: it has some guidelines about where the technique can be used and where it cannot: , | If your function needs to retrieve the tags including inherited tags | at the *current* entry, you can use the value of the variable | `org-scanner-tags' which will be much faster than getting the value | with `org-get-tags-at'. If your function gets properties with | `org-entry-properties' at the *current* entry, bind `org-trust-scanner-tags' | to t around the call to `org-entry-properties' to get the same speedup. | Note that if your function moves around to retrieve tags and properties at | a *different* entry, you cannot use these techniques. ` There are warnings that this variable is for internal dynamical scoping only, so I suspect you should not mess with the default. If your search can make the needed guarantees, then you can just wrap it in a let to get the speedup. Otherwise, it probably should be left alone. Nick 0003-Clocking-work-time-faster-filtering-of-clock-entries.patch Description: Binary data
