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 5:49 PM, João Távora wrote:
>> 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.
>>
>> So basically the same.  Cons and lambda's are cheap.
>
> The allocations itself are cheap but they put unnecessary pressure on
> the GC. The current version avoids this. This matters when scrolling
> through long candidate lists.

I've now benchmarked correctly (the previous benchmarks were completely
bogus)

;; current
(20.18251486 3 0.3386851209999975)
(19.646946365 3 0.3477981569999997)
(20.10486081 3 0.34829881900000004)

;; allocate extra cons and non-capturing function
(21.353061817 3 0.3557993659999994)
(21.124979874999998 3 0.361779398000003)
(21.009165698 3 0.36675408300000356)

;; allocate extra cons and closure function
(22.860096656 4 0.49817515299999826)
(22.091684587000003 4 0.4981799040000041)
(21.948139371 4 0.505049483999997)

;; allocate extra cons and string
(24.102438947 4 0.5202420680000017)

This was the test:

   (length joaot/test) ;; 44547 taken from the `read-char-by-name' table
   (let ((test (mapcar #'substring-no-properties joaot/test)))
     (garbage-collect)
     (benchmark-run 1
       (minibuffer--group-by #'mule--ucs-names-group
                             #'identity
                             test)
       (setq test nil)
       (garbage-collect)))

I really doubt this "matters when scrolling", as grouping would be
called once per filtering, and scrolling is just getting at each
candidate's transform.  If anything, if you take the last option, it
could _speed up_ scrolling (and the cost of some more initial latency).

>>> 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.
>
> The current version is proven in my packages and performs well. I don't
> see a point in replacing it to a more complicated and less efficient
> version.

I've told you the point.  A simpler minibuffer.el code, possibly with
less branching and logic, which might even be faster.

>>> 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.
>
> No it is not.

Look, this is just my opinion, you don't need to concur with it it, but
there's no need to say "no it's not" to an opinion.

> completion table authors. Furthermore I disagree with you that the
> minibuffer.el code is messy.

Surely, you jest, lol.  minibuffer-completion-help is a mess.  That's
ok!  Most of minibuffer.el is a mess.  There's no good code, in general,
so let's not get emotionally attached to ours.

> The suggestion of yours to mutate the candidates by putting a property
> instead of threading the group function through the call sites is much
> worse.

In what way?  Really, all other things being equal, in what way is
threading arguments through lots of functions, bloating the arglist,
implementations and the docstrings of said functions, calling group-fun
multiple times and in multiple places in minibuffer.el better than _not_
doing those things?

The only argument is if there was a terrible performance cost, which was
indeed the first and only real argument you advanced.  But I've not seen
evidence of that reality.

> Your version is not an improvement. Please read through the old
> discussion and consider my arguments. I also don't see a point in
> rediscussing this as long as you don't make a proposal which is a clear
> improvement.

Look, I get it that you oppose this suggestion and are happy with your
implementation.  That's fine it's your right to be.  But there's no
point in so aggresively gainsaying a suggestion while not offering any
material arguments.  APIs in master aren't meant to cristalize, they are
meant to be iterated.

João