Subject: Re: bug#46859: 28.0.50; [PATCH]: Add option to truncate long lines in xref.el

From: Theodor Thornhill <[email-masked]>
Date: Sun, 07 Mar 2021 21:16:18 +0100

Hi Dmitry,

> I just got around to testing this properly (sorry),

No worries

> and so far I've been 
> able to reproduce the slow behavior only when there are many matches in 
> the "big long line" file. I'm using a 500KB minified CSS as an example.
>
> When there are only a few matches, the search is relatively 
> instantaneous. So that's a weird mismatch with your reports. If you have 
> some details to add to reproduce the slowdown in the "few matches" case, 
> that could be helpful too.

Hmm. Theres always a possibility of a human error on my part during the
benchmarks, of course!

>
> I'm currently looking at the patch and trying to figure out whether we 
> could apply some smaller change, or a change not in xref--insert-xrefs 
> (which is relatively complex already) with the same benefits.
>

Yeah, I also wanted to not add too much to that function, but I couldn't
get improvements other places :)

> Also:
>
> - Could you explain the change to xref--collect-matches-1 in the most 
> recent patch? In my testing it doesn't move the needle at all, and it 
> seems unnecessary because neither Grep or Ripgrep report matches on the 
> same line separately with the current arguments that we pass to them. 
> But if we did... what's the idea? Skip all subsequent matches, no matter 
> if they're far or close?
>

Yeah, skipping subsequent matches yielded an improvement from ~1.09 to
~1.03 seconds, so not the biggest improvement, but it was consistent.
Thus I kept it. 

> - What do you think about making an effort to actually retain all the 
> matches in the output?

I see why we would want to do that, but as I mentioned in the last mail
I sent, these files are mostly "junk" anyways. However, it is probably
best to be able to retain them if we can. I just think speed should be
more important

> That would mean interpreting the xref-truncate-line-to value (or
> however the var could be renamed) as the maximum number of chars to
> render on the line *per match*. And if there is too much text between
> them, those parts can become "(truncated...)".  Your current
> implementation can cut off valid matches, and we probably want to
> preserve them if feasible. OTOH, the default value could go down to
> 200 with this approach.
>

Yeah, I had an implementation where I "snipped" between matches and
concatenated them together, but that still yielded too large a line for
my emacs on a 3 million char length file, so I scrapped that idea. I
guess it still is possible, though!

> What I mean is, we can provide the "fullest featured" default behavior, 
> one which never omits any valid matches and just truncated the line 
> context around them, and the users who want even faster searches (at the 
> cost of missing matches, esp. in find-replace scenarios) have something 
> else to customize too.

Yeah, I think this is the best approach too. Especially for grep users.

I'll still probably use
(add-to-list 
  'xref-search-program-alist
  '(ripgrep . "xargs -0 rg <C> -nH --no-messages -g '!*/' -e <R> -M 400
  --max-columns-preview | sort -t: -k1,1 -k2n,2"))

Or something to that effect anyways :)

--
Theo