[PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Emily Bourke]

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

Hi,

I found publishing when there were no changes to be slower than expected. Profiling showed me that `org-publish-cache-file-needs-publishing' was invoking the `after-find-file' hooks, which I don't think is necessary.

I've changed it to avoid doing that, by using `with-temp-buffer' and `insert-file-contents', and noticed a significant increase in speed.

Is there any reason I'm missing for using `find-file-noselect' in this case?

Best wishes,
Emily Bourke

[-- Attachment #2: 0001-ox-publish.el-Speed-up-org-publish-cache-file-needs-.patch --]
[-- Type: application/octet-stream, Size: 2742 bytes --]

From 0ae9d031ec368ddfe6295dce76aafbd368c5eb25 Mon Sep 17 00:00:00 2001
From: Emily Bourke <undergroundquizscene@gmail.com>
Date: Fri, 1 Jan 2021 18:47:45 +0000
Subject: [PATCH] ox-publish.el: Speed up
 org-publish-cache-file-needs-publishing

* lisp/ox-publish.el (org-publish-cache-file-needs-publishing): Use
`with-temp-buffer' with `insert-file-contents' instead of
`find-file-noselect'.  This avoids running the `after-find-file' hook,
which can make it significantly faster.

TINYCHANGE
---
 lisp/ox-publish.el | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/lisp/ox-publish.el b/lisp/ox-publish.el
index 7bb2fed6e..e967286cf 100644
--- a/lisp/ox-publish.el
+++ b/lisp/ox-publish.el
@@ -1290,29 +1290,26 @@ the file including them will be republished as well."
 	 (org-inhibit-startup t)
 	 included-files-ctime)
     (when (equal (file-name-extension filename) "org")
-      (let ((visiting (find-buffer-visiting filename))
-	    (buf (find-file-noselect filename))
-	    (case-fold-search t))
-	(unwind-protect
-	    (with-current-buffer buf
-	      (goto-char (point-min))
-	      (while (re-search-forward "^[ \t]*#\\+INCLUDE:" nil t)
-		(let ((element (org-element-at-point)))
-		  (when (eq 'keyword (org-element-type element))
-		    (let* ((value (org-element-property :value element))
-			   (filename
-			    (and (string-match "\\`\\(\".+?\"\\|\\S-+\\)" value)
-				 (let ((m (org-strip-quotes
-					   (match-string 1 value))))
-				   ;; Ignore search suffix.
-				   (if (string-match "::.*?\\'" m)
-				       (substring m 0 (match-beginning 0))
-				     m)))))
-		      (when filename
-			(push (org-publish-cache-ctime-of-src
-			       (expand-file-name filename))
-			      included-files-ctime)))))))
-	  (unless visiting (kill-buffer buf)))))
+      (let ((case-fold-search t))
+	(with-temp-buffer
+	  (insert-file-contents filename)
+	  (goto-char (point-min))
+	  (while (re-search-forward "^[ \t]*#\\+INCLUDE:" nil t)
+	    (let ((element (org-element-at-point)))
+	      (when (eq 'keyword (org-element-type element))
+		(let* ((value (org-element-property :value element))
+		       (filename
+			(and (string-match "\\`\\(\".+?\"\\|\\S-+\\)" value)
+			     (let ((m (org-strip-quotes
+				       (match-string 1 value))))
+			       ;; Ignore search suffix.
+			       (if (string-match "::.*?\\'" m)
+				   (substring m 0 (match-beginning 0))
+				 m)))))
+		  (when filename
+		    (push (org-publish-cache-ctime-of-src
+			   (expand-file-name filename))
+			  included-files-ctime)))))))))
     (or (null pstamp)
 	(let ((ctime (org-publish-cache-ctime-of-src filename)))
 	  (or (time-less-p pstamp ctime)
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Kyle Meyer]

Thank you for the patch.

Emily Bourke writes:

> I found publishing when there were no changes to be slower than
> expected. Profiling showed me that
> `org-publish-cache-file-needs-publishing' was invoking the
> `after-find-file' hooks, which I don't think is necessary.
>
> I've changed it to avoid doing that, by using `with-temp-buffer' and
> `insert-file-contents', and noticed a significant increase in speed.
>
> Is there any reason I'm missing for using `find-file-noselect' in this
> case?

Nothing jumps out to me.  For large files that are already visited, I
suppose find-file-noselect returning an existing buffer can be faster,
so relevant factors would include how many Org files a project has, how
large they are, and how many of those are visited in the current
session.  My guess is that using with-temp-buffer and
insert-file-contents would be a net gain, though that gain would be
narrowed some if the temporary buffer was put into org-mode rather than
kept in fundamental-mode (more below).

> Subject: [PATCH] ox-publish.el: Speed up
>  org-publish-cache-file-needs-publishing
>
> * lisp/ox-publish.el (org-publish-cache-file-needs-publishing): Use
> `with-temp-buffer' with `insert-file-contents' instead of
> `find-file-noselect'.  This avoids running the `after-find-file' hook,
> which can make it significantly faster.

This reads to me like after-find-file is the hook itself.  Perhaps
something like this would be clearer: "... avoids calling
after-find-file and running find-file-hook, ...".

> diff --git a/lisp/ox-publish.el b/lisp/ox-publish.el
> index 7bb2fed6e..e967286cf 100644
> --- a/lisp/ox-publish.el
> +++ b/lisp/ox-publish.el
> @@ -1290,29 +1290,26 @@ the file including them will be republished as well."
>  	 (org-inhibit-startup t)
>  	 included-files-ctime)
>      (when (equal (file-name-extension filename) "org")
> -      (let ((visiting (find-buffer-visiting filename))
> -	    (buf (find-file-noselect filename))
> -	    (case-fold-search t))
> -	(unwind-protect
> -	    (with-current-buffer buf
> -	      (goto-char (point-min))
> -	      (while (re-search-forward "^[ \t]*#\\+INCLUDE:" nil t)
> -		(let ((element (org-element-at-point)))
> -		  (when (eq 'keyword (org-element-type element))
> -		    (let* ((value (org-element-property :value element))
> -			   (filename
> -			    (and (string-match "\\`\\(\".+?\"\\|\\S-+\\)" value)
> -				 (let ((m (org-strip-quotes
> -					   (match-string 1 value))))
> -				   ;; Ignore search suffix.
> -				   (if (string-match "::.*?\\'" m)
> -				       (substring m 0 (match-beginning 0))
> -				     m)))))
> -		      (when filename
> -			(push (org-publish-cache-ctime-of-src
> -			       (expand-file-name filename))
> -			      included-files-ctime)))))))
> -	  (unless visiting (kill-buffer buf)))))
> +      (let ((case-fold-search t))
> +	(with-temp-buffer
> +	  (insert-file-contents filename)
> +	  (goto-char (point-min))

The goto-char call can be dropped now because insert-file-contents inserts
after point.

Unlike the previous code, this doesn't activate org-mode in the buffer.
That gives a speedup.  And I don't spot any code downstream that depends
on the major mode being org-mode, so it's probably safe, though perhaps
there's a subtle change in behavior here (e.g., related to syntax
table).

If org-mode isn't called, the org-inhibit-startup binding above could be
dropped.

> +	  (while (re-search-forward "^[ \t]*#\\+INCLUDE:" nil t)
> +	    (let ((element (org-element-at-point)))
> +	      (when (eq 'keyword (org-element-type element))
> +		(let* ((value (org-element-property :value element))
> +		       (filename
> +			(and (string-match "\\`\\(\".+?\"\\|\\S-+\\)" value)
> +			     (let ((m (org-strip-quotes
> +				       (match-string 1 value))))
> +			       ;; Ignore search suffix.
> +			       (if (string-match "::.*?\\'" m)
> +				   (substring m 0 (match-beginning 0))
> +				 m)))))
> +		  (when filename
> +		    (push (org-publish-cache-ctime-of-src
> +			   (expand-file-name filename))

This introduces a regression.  With the previous code, the
find-file-noselect call led to default-directory being set to the Org
file's directory, and then this expand-file call on the included file
was relative to that.  With the new code, default-directory isn't
changed, so it points to a non-existing or incorrect file unless the
current default-directory and the Org file's happen to match.

> +			  included-files-ctime)))))))))

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Emily Bourke]

Thanks for the feedback!

> Nothing jumps out to me. For large files that are already visited, I
> suppose find-file-noselect returning an existing buffer can be faster,
> so relevant factors would include how many Org files a project has, how
> large they are, and how many of those are visited in the current
> session. My guess is that using with-temp-buffer and
> insert-file-contents would be a net gain, though that gain would be
> narrowed some if the temporary buffer was put into org-mode rather than
> kept in fundamental-mode (more below).

I'll do some testing with some large org files and see how things compare – it might be worth switching to the buffer for the file if there is one already.

> This reads to me like after-find-file is the hook itself. Perhaps
> something like this would be clearer: "... avoids calling
> after-find-file and running find-file-hook, ...".

Ah yes, I had misunderstood – I'll rephrase it.

> The goto-char call can be dropped now because insert-file-contents inserts
> after point.

I follow, will remove.

> Unlike the previous code, this doesn't activate org-mode in the buffer.
> That gives a speedup. And I don't spot any code downstream that depends
> on the major mode being org-mode, so it's probably safe, though perhaps
> there's a subtle change in behavior here (e.g., related to syntax
> table).
>
> If org-mode isn't called, the org-inhibit-startup binding above could be
> dropped.

Yeah, if you're worried about it I could try manually activating org mode in the temp buffer – I'm not confident I could predict any problems there might be from not activating it.

> This introduces a regression. With the previous code, the
> find-file-noselect call led to default-directory being set to the Org
> file's directory, and then this expand-file call on the included file
> was relative to that. With the new code, default-directory isn't
> changed, so it points to a non-existing or incorrect file unless the
> current default-directory and the Org file's happen to match.

Ah, I hadn't noticed this – I'll change it to set default-directory manually.

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Bastien]

Hi Emily,

thanks for the patch and sorry to reactivate this old thread.

Did you find time to make the tests and, perhaps, to update the patch?

Please let us know - thanks!

-- 
 Bastien

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Emily Bourke]

Hi Bastien,

> thanks for the patch and sorry to reactivate this old thread.

No problem, thanks for getting in touch - the reminder is appreciated.

> Did you find time to make the tests and, perhaps, to update the patch?

I'm afraid I haven't had a chance to look at this any further since my last email. I'll try to find some time this week.

Best wishes,
Emily

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Bastien]

Hi Emily,

sorry to bump an old thread...

Emily Bourke <undergroundquizscene@protonmail.com> writes:

> I'm afraid I haven't had a chance to look at this any further since
> my last email. I'll try to find some time this week.

... did you have time to take a closer look at this?

We are working on preparing Org 9.5, hopefully releasing it quite
soon, it would be great if we can apply and test your patch before
that.

Thanks a lot!

-- 
 Bastien

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Emily Bourke]

Hi Bastien,

Thanks for getting in touch!

> ... did you have time to take a closer look at this?

I’m afraid I haven’t had any look at this since. However, I’ll have time tomorrow and later in the week to take a look – will that be early enough for the 9.5 release?

Best wishes,
Emily

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Bastien]

Hi Emily,

thanks for your quick feedback!

Emily Bourke <undergroundquizscene@protonmail.com> writes:

>> ... did you have time to take a closer look at this?
>
> I’m afraid I haven’t had any look at this since. However, I’ll have
> time tomorrow and later in the week to take a look – will that be
> early enough for the 9.5 release?

Tomorrow for sure, and probably next week will be okay too.

To be clear, I hope we can release 9.5 soon to be able to make it
included in the next Emacs 28.1 version.

Best,

-- 
 Bastien

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Emily Bourke]

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

Hi Bastien,

> Tomorrow for sure, and probably next week will be okay too.

Perfect, I’ll get back to you once I’ve taken a look tomorrow.

> To be clear, I hope we can release 9.5 soon to be able to make it
> included in the next Emacs 28.1 version.

Understood, thanks for the clarification.

Best wishes,
Emily

[-- Attachment #2: Type: text/html, Size: 366 bytes --]

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Emily Bourke]

Hi Bastien,

It looks like the same changes have been made separately and merged into master since my original email – see commit aa0fa8c75360d2aa491b9ae10e59d22de2aedc92 by Gustav Wikström.

Best wishes,
Emily

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Bastien]

Hi Emily,

Emily Bourke <undergroundquizscene@protonmail.com> writes:

> It looks like the same changes have been made separately and merged
> into master since my original email – see commit
> aa0fa8c75360d2aa491b9ae10e59d22de2aedc92 by Gustav Wikström.

oh, you're right!  Sorry I pinged you for nothing on this.

Gustav, for next time, don't forget to add the original author as the
author of the commit, thanks.

-- 
 Bastien

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Emily Bourke]

Hi Bastien,

> oh, you're right! Sorry I pinged you for nothing on this.

No worries!

Best wishes,
Emily

Fwd: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Gustav Wikström]

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

FYI. It seems we fixed the same issue half a year apart by chance..!

Maybe if we had a forge or smth this would have been solved already by Emily's patch beginning 2021. One can always wish, right? ;-)

Best
Gustav

________________________________
From: Bastien <bzg@gnu.org>
Sent: Monday, September 27, 2021 10:48 PM
To: Gustav Wikström
Subject: Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

Gustav Wikström <gustav@whil.se> writes:

> Aha, hmm... I don't remember commiting any other persons code. I
> honestly think it's just me fixing the same thing thing that
> apparently already had a patch without me knowing it!

I'm glad to read this :) - thanks, then!

--
 Bastien

[-- Attachment #2: Type: text/html, Size: 2170 bytes --]

Re: [PATCH] ox-publish.el: Speed up org-publish-cache-file-needs-publishing

[Dr. Arne Babenhauserheide]

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


Kyle Meyer <kyle@kyleam.com> writes:

> Nothing jumps out to me.  For large files that are already visited, I
> suppose find-file-noselect returning an existing buffer can be faster,
> so relevant factors would include how many Org files a project has, how
> large they are, and how many of those are visited in the current
> session.  My guess is that using with-temp-buffer and
> insert-file-contents would be a net gain, though that gain would be
> narrowed some if the temporary buffer was put into org-mode rather than
> kept in fundamental-mode (more below).

If you want to test a rather complex setup, you can try my website build:

hg clone http://hg.sr.ht/~arnebab/draketo; cd draketo/; autoreconf -i; ./configure ; time make

It takes around 3 minutes to build on my machine.

(hg is Mercurial)

Best wishes,
Arne
-- 
Unpolitisch sein
heißt politisch sein
ohne es zu merken

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1125 bytes --]