[PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Morgan Smith]

* lisp/org.el (org-cached-entry-get): Rewrite in terms
`org-entry-get'.  Obsolete in favor of `org-entry-get'.
(org-make-tags-matcher): Replace uses of `org-cached-entry-get' with
`org-entry-get'.
---
Hello!

All tests pass.

I don't think we can justify the existence of this function but let me know if
I'm wrong.

The caching mechanism used here is likely to cause hard to diagnose issues.

All of the logic here already exists in `org-entry-get'.

This function is mentioned in very few commits unlike its more popular sibling.

Thanks,

Morgan

 lisp/org.el | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index c91029c7f..80ffeeccf 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11480,21 +11480,10 @@ are also TODO tasks."
 
 (defalias 'org-tags-sparse-tree 'org-match-sparse-tree)
 
-(defvar org-cached-props nil)
 (defun org-cached-entry-get (pom property)
-  (if (or (eq t org-use-property-inheritance)
-	  (and (stringp org-use-property-inheritance)
-	       (let ((case-fold-search t))
-		 (string-match-p org-use-property-inheritance property)))
-	  (and (listp org-use-property-inheritance)
-	       (member-ignore-case property org-use-property-inheritance)))
-      ;; Caching is not possible, check it directly.
-      (org-entry-get pom property 'inherit)
-    ;; Get all properties, so we can do complicated checks easily.
-    (cdr (assoc-string property
-		       (or org-cached-props
-			   (setq org-cached-props (org-entry-properties pom)))
-		       t))))
+  (org-entry-get pom property 'selective))
+
+(make-obsolete 'org-cached-entry-get "use `org-entry-get' instead." "9.7")
 
 (defun org-global-tags-completion-table (&optional files)
   "Return the list of all tags in all agenda buffer/files.
@@ -11670,7 +11659,7 @@ See also `org-scan-tags'."
 				   ("CATEGORY"
 				    '(org-get-category (point)))
 				   ("TODO" 'todo)
-				   (p `(org-cached-entry-get nil ,p))))
+				   (p `(org-entry-get (point) ,p 'selective))))
 			     ;; Determine operand (aka. property
 			     ;; value).
 			     (pv (match-string 8 term))
@@ -11707,7 +11696,7 @@ See also `org-scan-tags'."
 	      (setq term rest)))
 	  (push `(and ,@tagsmatcher) orlist)
 	  (setq tagsmatcher nil))
-	(setq tagsmatcher `(progn (setq org-cached-props nil) (or ,@orlist)))))
+	(setq tagsmatcher `(or ,@orlist))))
 
     ;; Make the TODO matcher.
     (when (org-string-nw-p todomatch)
-- 
2.41.0

Re: [PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Ihor Radchenko]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

> * lisp/org.el (org-cached-entry-get): Rewrite in terms
> `org-entry-get'.  Obsolete in favor of `org-entry-get'.
> (org-make-tags-matcher): Replace uses of `org-cached-entry-get' with
> `org-entry-get'.
> ---
> Hello!
>
> All tests pass.
>
> All of the logic here already exists in `org-entry-get'.
>
> This function is mentioned in very few commits unlike its more popular sibling.

This function should yield speedups when matching special properties
like "CLOCKSUM", "CLOCKSUM_T", "TIMESTAMP", or "TIMESTAMP_IA".

For example, when the requested match tests these properties multiple
times.

> I don't think we can justify the existence of this function but let me know if
> I'm wrong.
>
> The caching mechanism used here is likely to cause hard to diagnose issues.

If you encountered such issue, please let me know.
Otherwise, I do not see any reason to remove `org-cached-entry-get'.

We need a real-life justification, not a theoretical one.

Canceled.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>

Re: [PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Morgan Smith]

Ihor Radchenko <yantar92@posteo.net> writes:

>
>
> This function should yield speedups when matching special properties
> like "CLOCKSUM", "CLOCKSUM_T", "TIMESTAMP", or "TIMESTAMP_IA".
>
> For example, when the requested match tests these properties multiple
> times.
>
> We need a real-life justification, not a theoretical one.
>
> Canceled.

I agree with all your points and I'm sorry for sending noise.  However, I think
I actually found a real-life justification.

It turns out that by replacing `org-cached-entry-get' with
`org-entry-get' the performance of my `org-clock-sum' calls got much
better for my specific use case.  Due to benchmarking with my local
changes in place (sorry), I accidentally attributed this performance
increase to byte-compiling the return of `org-make-tags-matcher'.

These numbers are also with my `org-clock-sum' rewrite patch applied.
They are in a 3M file of almost exclusivly clocking data.  The filters I
use are "CATEGORY={blah}" for one clock table and "-ignore-ITEM={foo}"
for 9 others.  

org-cached-entry-get
1st run: 26.868990287
2nd run: 16.043983143

org-entry-get
1st run: 18.209056578
2nd run: 5.003186764

Re: [PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

>> We need a real-life justification, not a theoretical one.
> ...
> I agree with all your points and I'm sorry for sending noise.  However, I think
> I actually found a real-life justification.
>
> It turns out that by replacing `org-cached-entry-get' with
> `org-entry-get' the performance of my `org-clock-sum' calls got much
> better for my specific use case.  Due to benchmarking with my local
> changes in place (sorry), I accidentally attributed this performance
> increase to byte-compiling the return of `org-make-tags-matcher'.
>
> These numbers are also with my `org-clock-sum' rewrite patch applied.
> They are in a 3M file of almost exclusivly clocking data.  The filters I
> use are "CATEGORY={blah}" for one clock table and "-ignore-ITEM={foo}"
> for 9 others.  

I got similar results using "-ignore-ITEM={foo}" :match.
The reason behind the slowdown is the fact that `org-cached-entry-get'
forces calculating _all_ the possible properties. In my case, this leads
to 10x slowdowns due to custom blocker function when calculating BLOCKED
property.

I thus conclude that your patch makes sense, and we can obsolete
`org-cached-entry-get', especially since atomic property lookups are
partially cached on the latest Org mode via org-element-cache
mechanisms.

However, please move the obsolete function definition to org-compat
instead of removing it completely. We do it to avoid unexpected breakage
for people and libraries who happen to use this public function.

Also, with the old approach, if you observe slowdowns, you likely have
some property being calculated slowly (like BLOCKED in my case). Do you
happen to know which property is it for your setup?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>

Re: [PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

Ihor Radchenko <yantar92@posteo.net> writes:
>
> However, please move the obsolete function definition to org-compat
> instead of removing it completely. We do it to avoid unexpected breakage
> for people and libraries who happen to use this public function.

Done.  See attached

> Also, with the old approach, if you observe slowdowns, you likely have
> some property being calculated slowly (like BLOCKED in my case). Do you
> happen to know which property is it for your setup?

According to my profiler, I think it's using 30% of the CPU time during
my custom org-clock-sum just to get ITEM.  I suppose it's because it
thinks it has to grab and cache everything when all I'm after is ITEM.
I don't see anything else that looks suspicious in the profiler so I
suspect you're seeing a much worse case then I am.  I'll copy paste my
previous performance numbers here again just so you can see my slowdown
is only between 1.5x and 3x.


org-cached-entry-get
1st run: 26.868990287
2nd run: 16.043983143

org-entry-get
1st run: 18.209056578
2nd run: 5.003186764



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Obsolete-org-cached-entry-get-in-favor-of-org-entry-.patch --]
[-- Type: text/x-patch, Size: 3679 bytes --]

From 5d9cef1250ef1eb656b84d3168ebfecb0e9c9c5c Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Wed, 1 May 2024 12:36:40 -0400
Subject: [PATCH] Obsolete `org-cached-entry-get' in favor of `org-entry-get'

We have a better performing cache mechanism in `org-entry-get'.

* lisp/org.el (org-make-tags-matcher): Replace uses of
`org-cached-entry-get' with `org-entry-get'.
(org-cached-entry-get): Move to ...
* lisp/org-compat.el (org-cached-entry-get): ... here.  Obsolete in
favor of `org-entry-get'.
---
 lisp/org-compat.el | 20 ++++++++++++++++++++
 lisp/org.el        | 20 ++------------------
 2 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/lisp/org-compat.el b/lisp/org-compat.el
index 11eb905ee..b73cfc910 100644
--- a/lisp/org-compat.el
+++ b/lisp/org-compat.el
@@ -649,6 +649,26 @@ Counting starts at 1."
 (define-obsolete-variable-alias 'org-plantuml-executable-args 'org-plantuml-args
   "Org 9.6")
 
+(defvar org-cached-props nil)
+(defun org-cached-entry-get (pom property)
+  (if (or (eq t org-use-property-inheritance)
+	  (and (stringp org-use-property-inheritance)
+	       (let ((case-fold-search t))
+		 (string-match-p org-use-property-inheritance property)))
+	  (and (listp org-use-property-inheritance)
+	       (member-ignore-case property org-use-property-inheritance)))
+      ;; Caching is not possible, check it directly.
+      (org-entry-get pom property 'inherit)
+    ;; Get all properties, so we can do complicated checks easily.
+    (cdr (assoc-string property
+		       (or org-cached-props
+			   (setq org-cached-props (org-entry-properties pom)))
+		       t))))
+
+(make-obsolete 'org-cached-entry-get
+               "Performs badly.  Instead use `org-entry-get' with the argument INHERIT set to `selective'"
+               "9.7")
+
 (defconst org-latex-line-break-safe "\\\\[0pt]"
   "Linebreak protecting the following [...].
 
diff --git a/lisp/org.el b/lisp/org.el
index 2d1a2055f..36b52b0ab 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11480,22 +11480,6 @@ are also TODO tasks."
 
 (defalias 'org-tags-sparse-tree 'org-match-sparse-tree)
 
-(defvar org-cached-props nil)
-(defun org-cached-entry-get (pom property)
-  (if (or (eq t org-use-property-inheritance)
-	  (and (stringp org-use-property-inheritance)
-	       (let ((case-fold-search t))
-		 (string-match-p org-use-property-inheritance property)))
-	  (and (listp org-use-property-inheritance)
-	       (member-ignore-case property org-use-property-inheritance)))
-      ;; Caching is not possible, check it directly.
-      (org-entry-get pom property 'inherit)
-    ;; Get all properties, so we can do complicated checks easily.
-    (cdr (assoc-string property
-		       (or org-cached-props
-			   (setq org-cached-props (org-entry-properties pom)))
-		       t))))
-
 (defun org-global-tags-completion-table (&optional files)
   "Return the list of all tags in all agenda buffer/files.
 Optional FILES argument is a list of files which can be used
@@ -11670,7 +11654,7 @@ See also `org-scan-tags'."
 				   ("CATEGORY"
 				    '(org-get-category (point)))
 				   ("TODO" 'todo)
-				   (p `(org-cached-entry-get nil ,p))))
+				   (p `(org-entry-get (point) ,p 'selective))))
 			     ;; Determine operand (aka. property
 			     ;; value).
 			     (pv (match-string 8 term))
@@ -11707,7 +11691,7 @@ See also `org-scan-tags'."
 	      (setq term rest)))
 	  (push `(and ,@tagsmatcher) orlist)
 	  (setq tagsmatcher nil))
-	(setq tagsmatcher `(progn (setq org-cached-props nil) (or ,@orlist)))))
+	(setq tagsmatcher `(or ,@orlist))))
 
     ;; Make the TODO matcher.
     (when (org-string-nw-p todomatch)
-- 
2.41.0

Re: [PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:
>> Also, with the old approach, if you observe slowdowns, you likely have
>> some property being calculated slowly (like BLOCKED in my case). Do you
>> happen to know which property is it for your setup?
>
> According to my profiler, I think it's using 30% of the CPU time during
> my custom org-clock-sum just to get ITEM.  I suppose it's because it
> thinks it has to grab and cache everything when all I'm after is ITEM.

Not sure here. Getting ITEM is just a single regexp match.
May you share the profiler report? (M-x profiler-report-write-profile)

> I don't see anything else that looks suspicious in the profiler so I
> suspect you're seeing a much worse case then I am.  I'll copy paste my
> previous performance numbers here again just so you can see my slowdown
> is only between 1.5x and 3x.

In my case it was more like 10x :)

> * lisp/org.el (org-make-tags-matcher): Replace uses of
> `org-cached-entry-get' with `org-entry-get'.
> (org-cached-entry-get): Move to ...
> * lisp/org-compat.el (org-cached-entry-get): ... here.  Obsolete in
> favor of `org-entry-get'.

Applied, onto main, after adding missing declare statements to pacify
the compiler.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=646f6ec13

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>

Re: [PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Morgan Smith]

Ihor Radchenko <yantar92@posteo.net> writes:

> Morgan Smith <morgan.j.smith@outlook.com> writes:
>>> Also, with the old approach, if you observe slowdowns, you likely have
>>> some property being calculated slowly (like BLOCKED in my case). Do you
>>> happen to know which property is it for your setup?
>>
>> According to my profiler, I think it's using 30% of the CPU time during
>> my custom org-clock-sum just to get ITEM.  I suppose it's because it
>> thinks it has to grab and cache everything when all I'm after is ITEM.
>
> Not sure here. Getting ITEM is just a single regexp match.
> May you share the profiler report? (M-x profiler-report-write-profile)

I'm experiencing some bugs with `profiler-find-profile'.  Might be
related to all the type changes that happened recently in Emacs master
(I like to run on the bleeding edge so I can experience as much pain and
frustration as possible).

Anyways, it's not just a regexp match since `org-cached-entry-get' tells
`org-entry-properties' to get everything.  We can see here almost all of
the slowdown for me occurs because of `org-element-context' which is
called in the `find-ts' lambda that search's for TIMESTAMP and
TIMESTAMP_IA.

8780  33%                    - org-cached-entry-get
8780  33%                     - org-entry-properties
8696  32%                      - #<byte-code-function 775>
8236  31%                       + org-element-context


The comment written just above `org-element-properties-map' says this:

#+BEGIN_SRC elisp
;; There is purposely no function like `org-element-properties' that
;; returns a list of properties.  Such function would tempt the users
;; to (1) run it, creating a whole new list; (2) filter over that list
;; - the process requiring a lot of extra consing, adding a load onto
;; Emacs GC, memory used, and slowing things up as creating new lists
;; is not free for CPU.
#+END_SRC

This implies that the function `org-entry-properties' is just a bad
idea.  Although giving it an argument for WHICH does make it better.

It seems like it's only used in about 4 places in our codebase so if we
wanted to go further, we could potentially see more performance
enhancements if we obsolete this function as well.

I haven't really looked into it much though so I apologize if my
analyses is wrong.  Which it definitely could be.

Re: [PATCH] lisp/org.el: Obsolete `org-cached-entry-get' in favor of `org-entry-get'

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

> The comment written just above `org-element-properties-map' says this:
>
> #+BEGIN_SRC elisp
> ;; There is purposely no function like `org-element-properties' that
> ;; returns a list of properties.  Such function would tempt the users
> ;; to (1) run it, creating a whole new list; (2) filter over that list
> ;; - the process requiring a lot of extra consing, adding a load onto
> ;; Emacs GC, memory used, and slowing things up as creating new lists
> ;; is not free for CPU.
> #+END_SRC
>
> This implies that the function `org-entry-properties' is just a bad
> idea.  Although giving it an argument for WHICH does make it better.

That's my comment, so I cannot disagree.

> It seems like it's only used in about 4 places in our codebase so if we
> wanted to go further, we could potentially see more performance
> enhancements if we obsolete this function as well.
>
> I haven't really looked into it much though so I apologize if my
> analyses is wrong.  Which it definitely could be.

`org-entry-properties' is the only place where the logic to retrieve
`org-special-properties' is implemented. So, we cannot just obsolete it.
It should be a more extensive refactoring with factoring out the logic
for retrieving special property values.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>