Inlining functions

[cage2]

Hi @bastibe !

There is neither new features or bugs fixed in this patch, nor regressions I hope! :wink:

I just followed a suggestions about in-lining the simpler functions in the code.

I very superficially performed profiling on a couple of them and appears they are faster but not sure if the optimization is not negligible.

EDIT Just to give a glimpse:

(define-inline annotate-annotations-from-dump-inline (record)
  "Get the annotations field from an annotation list loaded from a
file."
  (inline-letevals (record)
    (inline-quote (nth 8 ,record))))

(defun annotate-annotations-from-dump (record)
  "Get the annotations field from an annotation list loaded from a
file."
  (nth 8 record))

(cl-loop repeat 100000 do (annotate-annotations-from-dump '(1 2 3 4 5 6 7 8 9 0))) ;  Elapsed time: => 0.15s

(cl-loop repeat 100000 do (annotate-annotations-from-dump-inline '(1 2 3 4 5 6 7 8 9 0))) ; => Elapsed time: 0.078s

In any case i have learnt something new about elisp! :)

Bye! C.

[bastibe]

That's is a cool technique! Thank you for sharing it with me! It's actually something I use in C++ quite often (constexpr, template macros...), but I wasn't aware that it's this easy to do in Elisp.

I am somewhat conflicted, however, if this is the correct course of action for annotate. In essence, it moves the overhead of a single function call from run-time to compile-time. Since our code is neither performance-critical, nor called particularly often in a hot loop, this probably has no perceptible impact on annotate's usage either way. On the other hand, it does add some (minor) complexity to the code base, and makes debugging slightly more difficult.

Since you are the maintainer of annotate.el, I'll leave this completely up to you. If I were doing a code review at work, I'd probably flag this as premature optimization. However, this is not a commercial code base, but instead our own playground! It is our privilege that we can do things just because they are cool (and absolutely, this is super cool!).

In my own code, I tend to value readability much higher than anything else. "Readability Counts", as the Zen of Python calls it. Performance optimization, however, is often opposed to readability. To this, I generally respond in two ways: Optimizing only the critical code paths that verifiably (!) need to be optimized at the expense of readability (and commenting on the trade-offs profusely), and otherwise making sure that my code is not so much "optimized" for performance, but at least avoids rampant "pessimization" (as Casey Muratori calls it). In that vain, this change is probably not one I would make.

However, I repeat that I'll leave this entirely up to you. It's a cool technique, it doesn't hurt the code base, and I learned something new from this pull request (thank you!). So I'd say you'd be completely justified in merging it despite that nagging code monkey in the back of my brain.

[cage2]

On Sat, Aug 05, 2023 at 02:08:33AM -0700, you wrote:

Hi Bastibe!

First, thanks a lot for such a detailed answer. I appreciate that you reserved some that you could spend doing your work, with your family or even hobbies to write this message.

That's is a cool technique! Thank you for sharing it with me! It's actually something I use in C++ quite often (constexpr, template macros...), but I wasn't aware that it's this easy to do in Elisp.

I think this feature is a someway recent addiction as I was not able to find this topics in my old, local copy of the documentation, I was totally ignorant about the chance to inline code in Elisp until- a few weeks ago- someone suggested to use it for small functions.

I think this patch had a lot of didactic value for me and my Elisp skills but as I failed, so far, to provide meaningful data to support such code changes I agree with you that this patch falls in the category of premature optimization. :)

But I also believe the code could have didactic value for other people that are looking for example of how to use 'define-inline', but it this benefit worth the changes and the diminished readability? Sometime i think it does, some other it does not! :)

So i think I am going to leave the patch here, i am going to try to perform some more accurate profiling and if i will be not able to get valid benchmark data in the next seven days I'll close this PR.

Thanks again for spending your time for reviewing my code and providing insightful comments!

Bye! C.

[cage2]

Hi @bastibe !

I tried to get a better benchmark procedure and the data seems to indicate that the gain in speed is negligible (no gain after rounding), so i am closing this PR.

For the record I paste here the code i used and the data i got:

Because I do not want to throw the baby with the bath water, i am going to file a new PR just with the function line-beginning-position replacing our home made code.

Bye! C.

(setq all-annotations-data (let* ((filename (annotate-actual-file-name)))
                                    (annotate-load-annotation-data t)))

(defun benchmark-fn ()
  (dolist (annotation-dump all-annotations-data)
    (let* ((annotations  (annotate-annotations-from-dump annotation-dump))
           (old-checksum (annotate-checksum-from-dump annotation-dump))
           (new-checksum (annotate-buffer-checksum)))
      (dolist (annotation annotations)
        (let ((start             (annotate-beginning-of-annotation annotation))
              (end               (annotate-ending-of-annotation    annotation))
              (annotation-string (annotate-annotation-string       annotation))
              (annotated-text    (annotate-annotated-text          annotation)))
          (list start end annotation-string annotated-text))))))

(/ (cl-reduce #'+
           (cl-loop repeat 10 collect
                    (let ((data (benchmark-run-compiled 100
                                  (benchmark-fn))))
                      (- (cl-first data) (cl-third data)))))
   10)

;;; inline 0.23s

;;; not inline 0.23s