Re: [PATCH] (icomplete-vertical-mode): Add support for affixations and, annotations

[João Távora]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 6/1/21 4:30 PM, João Távora wrote:
>>> The design of the `group-function` has been extensively discussed. The
>>> flag is used instead of a tuple for performance reasons. The computation
>>> of the transformed candidate string may require string allocations,
>>> which are expensive if performed for every candidate.
>> 
>> That's right, I see your point.
>> 
>> They could return a lambda, which is cheap to allocate, to defer the
>> calculation.  Or just nil or #'identity if it's supposed to stay
>> unchanged.
>> ...
>> The protocol is slightly simpler since it returns the same type every
>> time.  More importantly, this avoids threading group-fn down a bunch of
>> functions in minibuffer.el, where it seems to be adding an awful lot of
>> complexity.
>
> No, I am against changing the group function. I am using this efficient
> definition in my packages and I rely on the performance
> characteristics.

But do you have any evidence to suggest that making a cons and
allocating a function is detrimental to this??  I just tried this small
patch to the function you suggested as a potential hog, since it
iterates all the completions.

    diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
    index ec21b7b93b..32737ff1ce 100644
    --- a/lisp/minibuffer.el
    +++ b/lisp/minibuffer.el
    @@ -1450,11 +1450,15 @@ minibuffer--group-by
       "Group ELEMS by GROUP-FUN and sort groups by SORT-FUN."
       (let ((groups))
         (dolist (cand elems)
    -      (let* ((key (funcall group-fun cand nil))
    +      (let* ((pair (cons (funcall group-fun cand nil)
    +                         (lambda () (list "oh no a capture"
    +                                          cand))))
    +             (key (car pair))
                  (group (assoc key groups)))
             (if group
                 (setcdr group (cons cand (cdr group)))
    -          (push (list key cand) groups))))
    +          (push (list key cand) groups))
    +        (put-text-property 0 1 'transform (cdr pair) cand)))
         (setq groups (nreverse groups)
               groups (mapc (lambda (x)
                              (setcdr x (nreverse (cdr x))))

(length joaot/test); 44547 taken from the `read-char-by-name' table

(benchmark-run 10000000
  '(minibuffer--group-by #'mule--ucs-names-group ; group-fn of that table
                         #'identity
                         joaot/test))
                         
;; Before the patch
;;
;; (0.10353484400000001 0 0.0)
;; (0.08804328700000008 0 0.0)
;; (0.11191811499999996 0 0.0)
;;
;; After
;;
;; (0.09785707300000002 0 0.0)
;; (0.10107326400000005 0 0.0)
;; (0.09778669299999998 0 0.0)

So basically the same.  Cons and lambda's are cheap.

Also, for good measure, I also changed that function to allocate a 30
char string for each completion (instead of the lambda).  Also quite
snappy (though there's maybe an effect showing).

;; (0.10938493999999999 0 0.0)
;; (0.10871186500000007 0 0.0)
;; (0.11186114000000003 0 0.0)

> The current implementation is simple enough and also avoids allocation
> of the cons pair and the allocation of the lambda as in your proposal.
> The efficiency is crucial here.

I believe you, but I haven't seen any evidence (yet) to back up the
claim.  But instead of me whistling premature optimization song, feel
free to point me to some numbers or something.

> Furthermore I argue that the current implementation is simpler to
> understand for the completion table authors than your counter
> proposal, which introduces a deferred computation with the lambda.

It's like the affixation-function thing, I don't think it makes sense
for client code to do these IFs, much as I dont' think it makes sense
for them to do MAPCARs.

But to be clear, the main advantage here in my opinion is the cleanup in
minibuffer.el, which is somehwat messy in my opinion (not only from
group-fn's fault, of course).

> There was a long discussion with Stefan, Dmitry,  Juri and Eli where
> multiple different designs for the `group-function` have been considered
> and we settled on the current design.

It could have been discussed with your favourite divinity, for all I
care.  If I see something worthy of improvement for the next version,
I'm going to suggest it here.  Especially in the case of a new feature
in master. "It's been discussed with so and so" is not a worthless
argument, but it's not very valuable either.

João