patch 8.2.4823: concat more than 2 strings in :def function is inefficient

Problem:    Concatenating more than 2 strings in a :def function is
            inefficient.
Solution:   Add a count to the CONCAT instruction. (closes #10276)

src/proto/vim9instr.pro

@@ -62,6 +62,7 @@ int generate_LEGACY_EVAL(cctx_T *cctx, char_u *line);
 int generate_EXECCONCAT(cctx_T *cctx, int count);
 int generate_RANGE(cctx_T *cctx, char_u *range);
 int generate_UNPACK(cctx_T *cctx, int var_count, int semicolon);
+int generate_CONCAT(cctx_T *cctx, int count);
 int generate_cmdmods(cctx_T *cctx, cmdmod_T *cmod);
 int generate_undo_cmdmods(cctx_T *cctx);
 int generate_store_var(cctx_T *cctx, assign_dest_T dest, int opt_flags, int vimvaridx, int scriptvar_idx, int scriptvar_sid, type_T *type, char_u *name);

src/testdir/test_vim9_disassemble.vim

@@ -208,7 +208,7 @@ def Test_disassemble_redir_var()
         ' redir END\_s*' ..
         '\d LOAD $0\_s*' ..
         '\d REDIR END\_s*' ..
-        '\d CONCAT\_s*' ..
+        '\d CONCAT size 2\_s*' ..
         '\d STORE $0\_s*' ..
         '\d RETURN void',
         res)
@@ -883,7 +883,7 @@ def Test_disassemble_closure()
         'local ..= arg\_s*' ..
         '\d LOADOUTER level 1 $0\_s*' ..
         '\d LOAD arg\[-1\]\_s*' ..
-        '\d CONCAT\_s*' ..
+        '\d CONCAT size 2\_s*' ..
         '\d STOREOUTER level 1 $0\_s*' ..
         '\d RETURN void',
         res)
@@ -973,7 +973,7 @@ def Test_disassemble_call_default()
         '6 LOAD arg\[-2]\_s*' ..
         '\d LOAD arg\[-1]\_s*' ..
         '\d 2STRING stack\[-1]\_s*' ..
-        '\d\+ CONCAT\_s*' ..
+        '\d\+ CONCAT size 2\_s*' ..
         '\d\+ RETURN',
         res)
 enddef
@@ -1245,9 +1245,9 @@ def Test_disassemble_lambda()
         '\d PUSHS "X"\_s*' ..
         '\d LOAD arg\[-1\]\_s*' ..
         '\d 2STRING_ANY stack\[-1\]\_s*' ..
-        '\d CONCAT\_s*' ..
+        '\d CONCAT size 2\_s*' ..
         '\d PUSHS "X"\_s*' ..
-        '\d CONCAT\_s*' ..
+        '\d CONCAT size 2\_s*' ..
         '\d RETURN',
         instr)
 enddef
@@ -1432,7 +1432,7 @@ def Test_disassemble_for_loop_eval()
         '\d\+ LOAD $0\_s*' ..
         '\d\+ LOAD $2\_s*' ..
         '\d 2STRING_ANY stack\[-1\]\_s*' ..
-        '\d\+ CONCAT\_s*' ..
+        '\d\+ CONCAT size 2\_s*' ..
         '\d\+ STORE $0\_s*' ..
         'endfor\_s*' ..
         '\d\+ JUMP -> 5\_s*' ..
@@ -2142,7 +2142,7 @@ def Test_disassemble_execute()
         "execute 'help ' .. tag\\_s*" ..
         '\d\+ PUSHS "help "\_s*' ..
         '\d\+ LOAD $1\_s*' ..
-        '\d\+ CONCAT\_s*' ..
+        '\d\+ CONCAT size 2\_s*' ..
         '\d\+ EXECUTE 1\_s*' ..
         '\d\+ RETURN void',
         res)

src/version.c

@@ -746,6 +746,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    4823,
 /**/
     4822,
 /**/

src/vim9.h

@@ -152,7 +152,7 @@ typedef enum {
     ISN_COMPAREANY,
 
     // expression operations
-    ISN_CONCAT,
+    ISN_CONCAT,     // concatenate isn_arg.number strings
     ISN_STRINDEX,   // [expr] string index
     ISN_STRSLICE,   // [expr:expr] string slice
     ISN_LISTAPPEND, // append to a list, like add()

src/vim9cmds.c

@@ -2125,7 +2125,7 @@ compile_redir(char_u *line, exarg_T *eap, cctx_T *cctx)
 	    generate_instr_type(cctx, ISN_REDIREND, &t_string);
 
 	    if (lhs->lhs_append)
-		generate_instr_drop(cctx, ISN_CONCAT, 1);
+		generate_CONCAT(cctx, 2);
 
 	    if (lhs->lhs_has_index)
 	    {

src/vim9compile.c

@@ -988,14 +988,12 @@ compile_heredoc_string(char_u *str, int evalstr, cctx_T *cctx)
     if (evalstr && (p = (char_u *)strstr((char *)str, "`=")) != NULL)
     {
 	char_u	*start = str;
+	int	count = 0;
 
 	// Need to evaluate expressions of the form `=<expr>` in the string.
 	// Split the string into literal strings and Vim expressions and
 	// generate instructions to concatenate the literal strings and the
 	// result of evaluating the Vim expressions.
-	val = vim_strsave((char_u *)"");
-	generate_PUSHS(cctx, &val);
-
 	for (;;)
 	{
 	    if (p > start)
@@ -1003,7 +1001,7 @@ compile_heredoc_string(char_u *str, int evalstr, cctx_T *cctx)
 		// literal string before the expression
 		val = vim_strnsave(start, p - start);
 		generate_PUSHS(cctx, &val);
-		generate_instr_drop(cctx, ISN_CONCAT, 1);
+		count++;
 	    }
 	    p += 2;
 
@@ -1011,7 +1009,7 @@ compile_heredoc_string(char_u *str, int evalstr, cctx_T *cctx)
 	    if (compile_expr0(&p, cctx) == FAIL)
 		return FAIL;
 	    may_generate_2STRING(-1, TRUE, cctx);
-	    generate_instr_drop(cctx, ISN_CONCAT, 1);
+	    count++;
 
 	    p = skipwhite(p);
 	    if (*p != '`')
@@ -1029,11 +1027,14 @@ compile_heredoc_string(char_u *str, int evalstr, cctx_T *cctx)
 		{
 		    val = vim_strsave(start);
 		    generate_PUSHS(cctx, &val);
-		    generate_instr_drop(cctx, ISN_CONCAT, 1);
+		    count++;
 		}
 		break;
 	    }
 	}
+
+	if (count > 1)
+	    generate_CONCAT(cctx, count);
     }
     else
     {
@@ -2382,7 +2383,7 @@ compile_assignment(char_u *arg, exarg_T *eap, cmdidx_T cmdidx, cctx_T *cctx)
 
 	    if (*op == '.')
 	    {
-		if (generate_instr_drop(cctx, ISN_CONCAT, 1) == NULL)
+		if (generate_CONCAT(cctx, 2) == FAIL)
 		    goto theend;
 	    }
 	    else if (*op == '+')

src/vim9execute.c

@@ -119,6 +119,48 @@ ufunc_argcount(ufunc_T *ufunc)
     return ufunc->uf_args.ga_len + (ufunc->uf_va_name != NULL ? 1 : 0);
 }
 
+/*
+ * Create a new string from "count" items at the bottom of the stack.
+ * A trailing NUL is appended.
+ * When "count" is zero an empty string is added to the stack.
+ */
+    static int
+exe_concat(int count, ectx_T *ectx)
+{
+    int		idx;
+    int		len = 0;
+    typval_T	*tv;
+    garray_T	ga;
+
+    ga_init2(&ga, sizeof(char), 1);
+    // Preallocate enough space for the whole string to avoid having to grow
+    // and copy.
+    for (idx = 0; idx < count; ++idx)
+    {
+	tv = STACK_TV_BOT(idx - count);
+	if (tv->vval.v_string != NULL)
+	    len += (int)STRLEN(tv->vval.v_string);
+    }
+
+    if (ga_grow(&ga, len + 1) == FAIL)
+	return FAIL;
+
+    for (idx = 0; idx < count; ++idx)
+    {
+	tv = STACK_TV_BOT(idx - count);
+	ga_concat(&ga, tv->vval.v_string);
+	clear_tv(tv);
+    }
+
+    // add a terminating NUL
+    ga_append(&ga, NUL);
+
+    ectx->ec_stack.ga_len -= count - 1;
+    STACK_TV_BOT(-1)->vval.v_string = ga.ga_data;
+
+    return OK;
+}
+
 /*
  * Create a new list from "count" items at the bottom of the stack.
  * When "count" is zero an empty list is added to the stack.
@@ -3536,6 +3578,11 @@ exec_instructions(ectx_T *ectx)
 		}
 		break;
 
+	    case ISN_CONCAT:
+		if (exe_concat(iptr->isn_arg.number, ectx) == FAIL)
+		    goto theend;
+		break;
+
 	    // create a partial with NULL value
 	    case ISN_NEWPARTIAL:
 		if (GA_GROW_FAILS(&ectx->ec_stack, 1))
@@ -4343,20 +4390,6 @@ exec_instructions(ectx_T *ectx)
 		}
 		break;
 
-	    case ISN_CONCAT:
-		{
-		    char_u *str1 = STACK_TV_BOT(-2)->vval.v_string;
-		    char_u *str2 = STACK_TV_BOT(-1)->vval.v_string;
-		    char_u *res;
-
-		    res = concat_str(str1, str2);
-		    clear_tv(STACK_TV_BOT(-2));
-		    clear_tv(STACK_TV_BOT(-1));
-		    --ectx->ec_stack.ga_len;
-		    STACK_TV_BOT(-1)->vval.v_string = res;
-		}
-		break;
-
 	    case ISN_STRINDEX:
 	    case ISN_STRSLICE:
 		{
@@ -6083,7 +6116,10 @@ list_instructions(char *pfx, isn_T *instr, int instr_count, ufunc_T *ufunc)
 	    case ISN_ADDBLOB: smsg("%s%4d ADDBLOB", pfx, current); break;
 
 	    // expression operations
-	    case ISN_CONCAT: smsg("%s%4d CONCAT", pfx, current); break;
+	    case ISN_CONCAT:
+		smsg("%s%4d CONCAT size %lld", pfx, current,
+					    (varnumber_T)(iptr->isn_arg.number));
+		break;
 	    case ISN_STRINDEX: smsg("%s%4d STRINDEX", pfx, current); break;
 	    case ISN_STRSLICE: smsg("%s%4d STRSLICE", pfx, current); break;
 	    case ISN_BLOBINDEX: smsg("%s%4d BLOBINDEX", pfx, current); break;

src/vim9expr.c

@@ -2513,7 +2513,8 @@ compile_expr5(char_u **arg, cctx_T *cctx, ppconst_T *ppconst)
 		if (may_generate_2STRING(-2, FALSE, cctx) == FAIL
 			|| may_generate_2STRING(-1, FALSE, cctx) == FAIL)
 		    return FAIL;
-		generate_instr_drop(cctx, ISN_CONCAT, 1);
+		if (generate_CONCAT(cctx, 2) == FAIL)
+		    return FAIL;
 	    }
 	    else
 		generate_two_op(cctx, op);

src/vim9instr.c

@@ -471,6 +471,33 @@ generate_COMPARE(cctx_T *cctx, exprtype_T exprtype, int ic)
     return OK;
 }
 
+/*
+ * Generate an ISN_CONCAT instruction.
+ * "count" is the number of stack elements to join together and it must be
+ * greater or equal to one.
+ * The caller ensures all the "count" elements on the stack have the right type.
+ */
+    int
+generate_CONCAT(cctx_T *cctx, int count)
+{
+    isn_T	*isn;
+    garray_T	*stack = &cctx->ctx_type_stack;
+
+    RETURN_OK_IF_SKIP(cctx);
+
+    if (count < 1)
+	return FAIL;
+
+    if ((isn = generate_instr(cctx, ISN_CONCAT)) == NULL)
+	return FAIL;
+    isn->isn_arg.number = count;
+
+    // drop the argument types
+    stack->ga_len -= count - 1;
+
+    return OK;
+}
+
 /*
  * Generate an ISN_2BOOL instruction.
  * "offset" is the offset in the type stack.