2

I recently noticed an issue in man.el, in which sections in Man--sections are listed backwards. That is, the value is is something like:

(#("SEE ALSO" 0 8
   (face Man-overstrike))
 #("COPYRIGHT" 0 9
   (face Man-overstrike))
 #("REPORTING BUGS" 0 14
   (face Man-overstrike))
 #("AUTHOR" 0 6
   (face Man-overstrike))
 #("EXAMPLES" 0 8
   (face Man-overstrike))
 #("DESCRIPTION" 0 11
   (face Man-overstrike))
 #("SYNOPSIS" 0 8
   (face Man-overstrike))
 #("NAME" 0 4
   (face Man-overstrike)))

This is because Man-build-section-alist scans the buffer from (point-min) to (point-max), push-ing values onto Man--sections as it goes.

I wanted to change this behavior so that the sections are in the correct order, which matters to helm-mode.

However, changing the Man-build-section-alist function, then re-evaluating it doesn't do any good, because it's defined with defsubst instead of defun. defsubst inlines functions, which means that, when the file is byte-compiled, the function definition is directly inserted where the function is called. This means that redefining/evaluating the function doesn't work, since the bytecode is not changed.

defsubst offers a performance benefit, but is there really any reason to use it for a command like Man-build-section-alist? Most functions in man.el use defun, and I can't imagine that the performance benefit matters on hardware from the past 15 years.

Tianxiang Xiong
  • 3,878
  • 18
  • 28
  • IMHO, there is rarely a good reason to use defsubst nowadays. People sometimes think that a function (often a tiny one, but that makes no difference) will never need to be used by anyone (a user, using Lisp). They are almost always wrong in principle, and almost almost always wrong in practice. I think it comes from a shortsighted view of Lisp users, and perhaps from an ingrained notion of a separation between "developers" and "users" - a notion that some bring to Emacs from other kinds of programming, in other contexts (e.g., not for user interaction). -- Just one opinion. – Drew Sep 16 '16 at 04:16
  • 1
    If speed matters, use defmacro, not defsubst. If you need a function because it will be funcalled or applyed or mapped, use defun, not defsubst. Show me a case where defsubst is used and I'm pretty sure it should not be used. – Drew Sep 16 '16 at 04:20
  • I'm not sure defmacro should be used just for a performance boost. Macros are used to extend the language, and carry costs (e.g. more difficult to reason about and debug). – Tianxiang Xiong Sep 16 '16 at 05:39
  • @TianxangXiong: Agreed. I was contrasting it with defsubst. – Drew Sep 16 '16 at 15:00
  • @Drew "If speed matters, use defmacro, not defsubst" - that makes no sense, defmacro has all the drawbacks of defsubst and additionally the code will be more complicated by the multiple phases of evaluation (I agree that defsubst is usually a bad idea, typically premature optimization) – npostavs Sep 16 '16 at 15:47
  • @npostavs: Using a macro saves only a function call, yes. And that makes little difference nowadays. But saving a function call was the reason people used defsubst - that, combined with the ability to be invoked as a function (which defmacro lacks). And for that reason (alone), defmacro is better than defsubst. (I cannot edit my older comment to clarify it.) – Drew Sep 16 '16 at 16:01
  • 1
    FWIW, Emacs-25 comes with a new define-inline macro which can be used in those cases where performance is important. define-inline is a kind of halfway between defsubst and defmacro: it defines a function (i.e. funcallable) but you can optimize it when it's inlined. It's not as simple to use as defsubst, but results in more efficient code. – Stefan Sep 17 '16 at 19:54
  • Neat, but define-inline seems like a chore to use, with inline-quote, inline-letevals, and a whole bunch of other macros to go along with it. – Tianxiang Xiong Sep 17 '16 at 20:08
  • BTW, you might like to M-x report-emacs-bug saying that it's OK for a defsubst function to be inlined, but it should be de/re-inlined whenever its definition changes (i.e. Emacs should keep track of those inlinings and keep them uptodate). – Stefan Sep 17 '16 at 20:16

2 Answers2

3

FWIW, I disagree with a general ban on defsubst, but these two uses of defsubst in man.el seem like clear errors, indeed. Basically any function which contains a loop will spend much more time in its loop than entering/leaving the function itself, so using defsubst is just not appropriate in such a case.

Stefan
  • 26,404
  • 3
  • 48
  • 85
  • You disagree that defsubst is no longer useful, but you do not say which cases you find it useful for. (And no one has suggested "banning" it.) – Drew Sep 16 '16 at 16:03
  • It is useful in some cases in order to circumvent the low-efficiency of function calls. I.e. when the function is called very often, and when the runtime of the function is (usually) very short. – Stefan Sep 17 '16 at 19:56
  • I'd love to see an example, with some stats about performance. All I've seen so far just represents an inconvenience for someone who wants to actually use the defsubst as a real (runtime) function. I don't think I've seen a case where performance matters in decades. Probably such a case could be concocted, but I'd be curious to see a real such case. – Drew Sep 17 '16 at 21:06
  • @Drew: here's an example where defsubst is useful (it fixes the "recursive autoload" issue) – jfs Mar 15 '20 at 06:34
  • 1
    @jfs: they don't need to make it a defsubst in order for the whole definition to be put into the autoload file. They could use ;;;###autoload \n (progn (defun ...)) instead. – Stefan Mar 15 '20 at 13:41
0

I do not know why man.el uses defsubst—you'd have to ask whoever wrote the function—but I'm sure that it's not really needed. I agree with Drew: Nowadays you shouldn't use defsubst anymore.

But then you shouldn't patch or advice other libraries as well, unless absolutely necessary. And the inverse order of a list doesn't strike me as a good reason for advicing.

I'm not sure why Helm should require this variable to be in proper order, but you should always be able to point it to another variable that holds the reverse list.

But please do not use advices.

  • I certainly wasn't going to modify Helm to advise the the Man-build-sections-alist function. If anything, it's the Man function that should be modified to return values in proper order.. – Tianxiang Xiong Sep 16 '16 at 06:28
  • I was going to say something similar to lunaryorn: Why does the order matter? (But I disagree about "do not use advices", except I agree that it makes sense to minimize doing so.) – Drew Sep 16 '16 at 15:03
  • Why shouldn't the order matter? If I know that a collection is ordered, I won't have to sort it again. Besides, the push-nreverse idiom is common in Lisp. – Tianxiang Xiong Sep 17 '16 at 20:09