[vim/vim] Reduce number of calls to STRLEN() (Issue #14002)

John Marriott

unread,

Feb 9, 2024, 4:29:33 PM2/9/24

to vim/vim, Subscribed

There are numerous of calls to STRLEN() throughout the tree and I think that a lot of them can be avoided. This is especially true when calling the ml_get*() functions because the buf_T struct has a member (ml_line_len) which contains the length of the line.

To demonstrate one way it could be done I have put together the following patches.

Any feedback would be welcomed. If this is something that people believe is worth pursuing I am happy to keep working on it.

Cheers
John

normal.c.9.1.0089.patch
memline.pro.9.1.0089.patch
memline.c.9.1.0089.patch

—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Feb 9, 2024, 5:04:30 PM2/9/24

to vim/vim, Subscribed

Thanks. I think this makes sense.

I had a quick look at the patch:

@@ -5192,15 +5242,15 @@
 		    {
 			char_u	*name;
 			int	dialog_result;
+			size_t  len = STRLEN(_("Swap file \""));
 
 			name = alloc(STRLEN(fname)
-				+ STRLEN(_("Swap file \""))
+				+ len
 				+ STRLEN(_("\" already exists!")) + 5);
 			if (name != NULL)
 			{
 			    STRCPY(name, _("Swap file \""));
-			    home_replace(NULL, fname, name + STRLEN(name),
-								  1000, TRUE);
+			    home_replace(NULL, fname, name + len, 1000, TRUE);
 			    STRCAT(name, _("\" already exists!"));
 			}
 			dialog_result = do_dialog(VIM_WARNING,

I think here it is wrong. name + STRLEN(name) is not the same as name + len. It's missing the STRLEN(_("\" already exists!")) + 5 part.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

John Marriott

unread,

Feb 10, 2024, 3:57:38 PM2/10/24

to vim/vim, Subscribed

Here are some more patches.

eval.c.9.1.0095.patch
search.c.9.1.0095.patch
edit.c.9.1.0095.patch
drawline.c.9.1.0095.patch

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Feb 11, 2024, 10:33:03 AM2/11/24

to vim/vim, Subscribed

can you make a PR for this please?

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

John Marriott

unread,

Feb 13, 2024, 3:25:54 PM2/13/24

to vim...@googlegroups.com

--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

---
You received this message because you are subscribed to the Google Groups "vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/vim_dev/vim/vim/issues/14002/1937786687%40github.com.

Hi Christian,

I have made my first PR (#14029).

When I did so, I got a 9 failing checks which mostly appear to be failing tests, like this:
<snip>
command line..script /home/runner/work/vim/vim/src/testdir/runtest.vim[642]..function RunTheTest[57]..Test_prop_below_split_line[25]..StopVimInTerminal[17]..WaitForAssert[2]..<SNR>4_WaitForCommon[11]..<lambda>38 line 1: Expected 'finished' but got 'running'

Flaky test failed too often, giving up
</snip>

I'm not sure how to proceed with this. This is my first PR.

Any suggestions would be appreciated.

Cheers
John

Christian Brabandt

unread,

Feb 13, 2024, 4:00:15 PM2/13/24

to vim...@googlegroups.com

On Mi, 14 Feb 2024, John Marriott wrote:

> Hi Christian,
>
> I have made my first PR (#14029).

Thank you!

>
> When I did so, I got a 9 failing checks which mostly appear to be failing
> tests, like this:
> <snip>
> command line..script
> /home/runner/work/vim/vim/src/testdir/runtest.vim[642]..function RunTheTest[57]..Test_prop_below_split_line[25]..StopVimInTerminal[17]..WaitForAssert[2]..<SNR>4_WaitForCommon[11]..<lambda>38
> line 1: Expected 'finished' but got 'running'
> Flaky test failed too often, giving up
> </snip>
>
> I'm not sure how to proceed with this. This is my first PR.

No problem. If I check the CI on github, I see a few failures, including
a buffer-overflow (https://github.com/vim/vim/actions/runs/7891958995/job/21537397938?pr=14029):
,----
| READ of size 1 at 0x5020000abad3 thread T0
| #0 0x5610bb99642a in ___interceptor_strlen ??:?
| #1 0x5610bbaca046 in open_line /home/runner/work/vim/vim/src/change.c:1488:19
| #2 0x5610bbc064ea in ins_eol /home/runner/work/vim/vim/src/edit.c:5162:9
| #3 0x5610bbbf045d in edit /home/runner/work/vim/vim/src/edit.c:1230:10
| #4 0x5610bc01e110 in invoke_edit /home/runner/work/vim/vim/src/normal.c:7098:9
| #5 0x5610bbfd8fd9 in normal_cmd /home/runner/work/vim/vim/src/normal.c:949:5
| #6 0x5610bc8d1d18 in main_loop /home/runner/work/vim/vim/src/main.c
| #7 0x5610bc8cfc89 in vim_main2 /home/runner/work/vim/vim/src/main.c:895:5
| #8 0x5610bc8c825c in main /home/runner/work/vim/vim/src/main.c:441:12
| #9 0x7f4586229d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
| #10 0x7f4586229e3f in __libc_start_main csu/../csu/libc-start.c:392:3
| #1 0x5610bb97ef94 in _start ??:?
`----

This indicates that something serious got brocken. You may want to
compile with ASAN enabled and run the test suite on your side to find
out which test causes this. The fact that it is finished, could mean
that the process segfaulted, in which case it would be finished indeed
:)
I guess this may be the following test.
Test_prop_below_split_line(): Vim(call):E958: Job already finished @
command line..script
/home/runner/work/vim/vim/src/testdir/runtest.vim[607]..function
RunTheTest[57]..Test_prop_below_split_line[17]..VerifyScreenDump, line 35

Test_prop_below_split_line() sounds like it could invoke the open_line()
function but I am just guessing here.

If I check the test, line 17:

,----
| 1func Test_prop_below_split_line()
| 2 CheckRunVimInTerminal
| 3
| 4 let lines =<< trim END
| 5 vim9script
| 6 setline(1, ['one one one', 'two two two', 'three three three'])
| 7 prop_type_add('test', {highlight: 'Search'})
| 8 prop_add(2, 0, {
| 9 text: '└─ Virtual text below the 2nd line',
| 10 type: 'test',
| 11 text_align: 'below',
| 12 text_padding_left: 3
| 13 })
| 14 END
| 15 call writefile(lines, 'XscriptPropBelowSpitLine', 'D')
| 16 let buf = RunVimInTerminal('-S XscriptPropBelowSpitLine', #{rows: 8})
| 17 call term_sendkeys(buf, "2GA\<CR>xx")
| 18 call VerifyScreenDump(buf, 'Test_prop_below_split_line_1', {})
| 19
| 20 call term_sendkeys(buf, "\<Esc>:set number\<CR>")
| 21 call VerifyScreenDump(buf, 'Test_prop_below_split_line_2', {})
`----

Line 17 is the line where we append a <CR> to the buffer, so it could match.

Other than that, screendumps are unfortunately quite flaky, so it is
okay, if only 1 or 2 tests from the suite fail, but 6 is too much :(

So try to run the failing tests in your local environment and see if you
can debug what is going wrong. See the readme of the testsuite on how to
run only a single test:
https://github.com/vim/vim/blob/master/src/testdir/README.txt

Thanks,
Chris
--
Youth is a blunder, manhood a struggle, old age a regret.
-- Benjamin Disraeli, "Coningsby"

Christian Brabandt

unread,

Feb 21, 2024, 3:40:13 PM2/21/24

to vim/vim, Subscribed

Closed #14002 as not planned.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Feb 21, 2024, 3:40:14 PM2/21/24

to vim/vim, Subscribed

closing in favor of #14052

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward