New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(runtime): fix strace regex #12220
Conversation
@brammool do you see any value in this? |
Federico Mengozzi wrote:
I think this regex can be simplified with no side effects apart from
an improvement in performance.
When opening text file with long lines of numbers (bitstrings in my
case) the additional and optional `[0-9:.]* *` put unnecessary work on
the regex engine.
This is even more true in neovim, whose lua regex engine is not as
good as vim. Honestly I don't see any reason why the regex should
have the additional pattern. The two tests works fine with the
simplified version
Hmm, I'm not 100% sure this won't cause problems. execve() is also used
in C programs at least. To avoid changing the functionality and still
have nearly all of the performance improvement, we can first check for
a match with "execve(" and only then check for the current pattern:
elseif (line1 =~ 'execve(' && line1 =~ '[0-9:.]* *execve(') || line1 =~ '^__libc_start_main'
How about that?
…--
The Law of VIM:
For each member b of the possible behaviour space B of program P, there exists
a finite time t before which at least one user u in the total user space U of
program P will request b becomes a member of the allowed behaviour space B'
(B' <= B).
In other words: Sooner or later everyone wants everything as an option.
-- Vince Negri
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
|
@brammool I think that would definitely be better, however I'm not sure it's really required since the additional For example, with this config
a file with this line
it's already being recognised as That's why I was thinking we could either drop the https://github.com/vim/vim/blame/875feea6ce223462d55543735143d747dcaf4287/runtime/scripts.vim#L305 |
@brammool I think that would definitely be better, however I'm not
sure it's really required since the additional `[0-9:.]* *` is
optional. It's not really changing the result of the simpler regex.
For example, with this config `.vimrc`
```
filetype on
```
a file with this line
```
execve()
```
it's already being picked with as `strace` type.
Right, it appears this pattern was already faulty. I guess a "^" should
have been at the start. That way, if "execve" would be preceded by
something else than a timestamp it would not match. So it would need to
be:
```
elseif (line1 =~ 'execve(' && line1 =~ '^[0-9:.]* *execve(') || line1 =~ '^__libc_start_main'
```
To avoid false matches. This would still be fast.
…--
VOICE OVER: As the horrendous Black Beast lunged forward, escape for Arthur
and his knights seemed hopeless, when, suddenly ... the animator
suffered a fatal heart attack.
ANIMATOR: Aaaaagh!
VOICE OVER: The cartoon peril was no more ... The Quest for Holy Grail could
continue.
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
|
@brammool I changed the line as you suggested. I added a small test too. LMK if this works |
Codecov Report
@@ Coverage Diff @@
## master #12220 +/- ##
=======================================
Coverage 81.95% 81.95%
=======================================
Files 164 164
Lines 194112 194142 +30
Branches 43831 43836 +5
=======================================
+ Hits 159079 159117 +38
+ Misses 22195 22193 -2
+ Partials 12838 12832 -6
Flags with carried forward coverage won't be shown. Click here to find out more. see 31 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Problem: Strace filetype detection is expensive. Solution: Match with a cheap pattern first. (Federico Mengozzi, closes vim/vim#12220) vim/vim@6e5a9f9 Co-authored-by: Federico Mengozzi <19249682+fedemengo@users.noreply.github.com>
Problem: Strace filetype detection is expensive. Solution: Match with a cheap pattern first. (Federico Mengozzi, closes vim/vim#12220) vim/vim@6e5a9f9 Co-authored-by: Federico Mengozzi <19249682+fedemengo@users.noreply.github.com>
Problem: Strace filetype detection is expensive. Solution: Match with a cheap pattern first. (Federico Mengozzi, closes vim#12220)
Problem: Strace filetype detection is expensive. Solution: Match with a cheap pattern first. (Federico Mengozzi, closes vim/vim#12220) vim/vim@6e5a9f9 Co-authored-by: Federico Mengozzi <19249682+fedemengo@users.noreply.github.com> (cherry picked from commit 9808866)
) Problem: Strace filetype detection is expensive. Solution: Match with a cheap pattern first. (Federico Mengozzi, closes vim/vim#12220) vim/vim@6e5a9f9 Co-authored-by: Federico Mengozzi <19249682+fedemengo@users.noreply.github.com>
I think this regex can be simplified with no undesired effects.
When opening text file with long lines of numbers (bitstrings in my case) the additional and optional
[0-9:.]* *
put unnecessary work on the regex engine.Honestly I don't see any reason why the regex should have the additional pattern. The two tests works fine with the simplified version