bug#47711: bug#48841: bug#47711: bug#48841: bug#47711: [PATCH VERSION 2] Add new `completion-filter-completions` API and deferred highlighting

[Dmitry Gutov <dmitry@gutov.dev>]

On 27/10/2023 03:26, João Távora wrote:
> On Fri, Oct 27, 2023 at 1:11 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
>>> Also in the last iteration of the "yoyo" fido-vertical-mode test,
>>> results with my latest patch are quite a bit faster.
>>
>> Hmm, your latest (lazy-hilit-2023-v3.diff) improves the (completing-read
>> "" obarray) scenario a lot (over the original two 2023 solutions), but
>> not the the 'C-h v' scenario. I don't have an explanation.
> 
> The improvement was due to running string-match only once per completion,
> if you look at the changes to completion-pcm--all-completions.

Right. I just don't (didn't?) have an explanation for the difference in 
the improvement in performance (or the lack thereof) between the two 
scenarios.

> It could be this code doesn't kick in in the C-h v scenario.  Notice
> that that function already has some optimizations: for example, when
> the regexp match is performed by all-completions and its use of
> completion-regexp-alist, there's no way to get the regex match data
> to compute the score, so it'll have to be postponed to
> completion-pcm--hilit-commonality as in the v2.diff case.
> 
> Then again, that doesn't explain why in the C-h v scenario, the regression
> was fixed precisely by adjust that code that I was conjecturing might
> not kick in.

According to my print-debugging, the situation is the reverse: 
(completing-read "" obarray) goes the "The internal functions already 
obeyed" route (because obarray is not a function?), and the scenario 
that didn't get better (C-h v) goes down the clause "pattern has 
something interesting to match". Unless the input is empty, then it also 
goes down the first clause.

So it seems like we might misunderstand the reason why the former got 
faster. I see the check changed from

   (not (functionp table)

to

   (or (not (functionp table))
       (cl-loop for e in pattern never (stringp e)))

but that can't be that reason.

BTW, all-completion's docstring also says that a COLLECTION that is a 
function should itself handle completion-regexp-list, so we could try to 
rely on that too and drop the additional check. That's risky, though, so 
something for a later follow-up.

> Anyway, I think it's safer to say that my latest patch is at least as fast,
> sometimes faster, than the 2023 completion-filter-completions solution.

All other things equal, I also prefer a smaller change, and thank you 
for producing it. Functionally, too, it's almost equivalent to 
completion-filter-completions. So one could easily write a wrapper like 
that with the same performance.

But there are differences. The first is that the highlighter function 
takes one string as an argument instead of a collection. I mentioned 
this before, this will be much handier to use in company-capf.

Second, in Daniel's patch the "adjust metadata" function got a 
different, arguably better, calling convention. That's not dependent on 
the rest of the patch, so it can be considered separately.

Third, it made a principled stance to avoid altering the original 
strings, even the non-visual text properties. This approach could be 
adopted piecewise from Daniel's patch, especially if the performance 
ends up the same or insignificantly different in practical usage.

As for whether we migrate to the completion-filter-completions API, I 
don't have a strong opinion. If we still think that the next revision of 
the completion API will be radically different, then there is not much 
point in making medium-sized steps like that. OTOH, if we end up with 
the same API for the next decade and more, completion-filter-completions 
does look more meaningful, and more easily extensible (e.g. next we 
could add a pair (completion-scorer . fn) to its return value; and so 
on). And again, the implementation could be a simple wrapper at first.