To: vim_dev@googlegroups.com Subject: Patch 8.2.1819 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ Patch 8.2.1819 Problem: Vim9: Memory leak when using a closure. Solution: Compute the mininal refcount in the funcstack. Reenable disabled tests. Files: src/vim9execute.c, src/proto/vim9execute.pro, src/structs.h, src/eval.c, src/testdir/test_vim9_disassemble.vim, src/testdir/test_vim9_func.vim *** ../vim-8.2.1818/src/vim9execute.c 2020-10-07 19:08:00.009793481 +0200 --- src/vim9execute.c 2020-10-10 14:12:23.944818556 +0200 *************** *** 349,356 **** // Move them to the called function. if (funcstack == NULL) return FAIL; ! funcstack->fs_ga.ga_len = argcount + STACK_FRAME_SIZE ! + dfunc->df_varcount; stack = ALLOC_CLEAR_MULT(typval_T, funcstack->fs_ga.ga_len); funcstack->fs_ga.ga_data = stack; if (stack == NULL) --- 349,356 ---- // Move them to the called function. if (funcstack == NULL) return FAIL; ! funcstack->fs_var_offset = argcount + STACK_FRAME_SIZE; ! funcstack->fs_ga.ga_len = funcstack->fs_var_offset + dfunc->df_varcount; stack = ALLOC_CLEAR_MULT(typval_T, funcstack->fs_ga.ga_len); funcstack->fs_ga.ga_data = stack; if (stack == NULL) *************** *** 376,404 **** { tv = STACK_TV(ectx->ec_frame_idx + STACK_FRAME_SIZE + idx); ! // Do not copy a partial created for a local function. ! // TODO: This won't work if the closure actually uses it. But when ! // keeping it it gets complicated: it will create a reference cycle ! // inside the partial, thus needs special handling for garbage ! // collection. ! // For now, decide on the reference count. if (tv->v_type == VAR_PARTIAL && tv->vval.v_partial != NULL) { ! int i; for (i = 0; i < closure_count; ++i) ! { ! partial_T *pt = ((partial_T **)gap->ga_data)[gap->ga_len ! - closure_count + i]; ! ! if (tv->vval.v_partial == pt && pt->pt_refcount < 2) ! break; ! } ! if (i < closure_count) ! continue; } ! *(stack + argcount + STACK_FRAME_SIZE + idx) = *tv; tv->v_type = VAR_UNKNOWN; } --- 376,397 ---- { tv = STACK_TV(ectx->ec_frame_idx + STACK_FRAME_SIZE + idx); ! // A partial created for a local function, that is also used as a ! // local variable, has a reference count for the variable, thus ! // will never go down to zero. When all these refcounts are one ! // then the funcstack is unused. We need to count how many we have ! // so we need when to check. if (tv->v_type == VAR_PARTIAL && tv->vval.v_partial != NULL) { ! int i; for (i = 0; i < closure_count; ++i) ! if (tv->vval.v_partial == ((partial_T **)gap->ga_data)[ ! gap->ga_len - closure_count + i]) ! ++funcstack->fs_min_refcount; } ! *(stack + funcstack->fs_var_offset + idx) = *tv; tv->v_type = VAR_UNKNOWN; } *************** *** 427,432 **** --- 420,462 ---- } /* + * Called when a partial is freed or its reference count goes down to one. The + * funcstack may be the only reference to the partials in the local variables. + * Go over all of them, the funcref and can be freed if all partials + * referencing the funcstack have a reference count of one. + */ + void + funcstack_check_refcount(funcstack_T *funcstack) + { + int i; + garray_T *gap = &funcstack->fs_ga; + int done = 0; + + if (funcstack->fs_refcount > funcstack->fs_min_refcount) + return; + for (i = funcstack->fs_var_offset; i < gap->ga_len; ++i) + { + typval_T *tv = ((typval_T *)gap->ga_data) + i; + + if (tv->v_type == VAR_PARTIAL && tv->vval.v_partial != NULL + && tv->vval.v_partial->pt_funcstack == funcstack + && tv->vval.v_partial->pt_refcount == 1) + ++done; + } + if (done == funcstack->fs_min_refcount) + { + typval_T *stack = gap->ga_data; + + // All partials referencing the funcstack have a reference count of + // one, thus the funcstack is no longer of use. + for (i = 0; i < gap->ga_len; ++i) + clear_tv(stack + i); + vim_free(stack); + vim_free(funcstack); + } + } + + /* * Return from the current function. */ static int *** ../vim-8.2.1818/src/proto/vim9execute.pro 2020-08-12 21:34:43.266489468 +0200 --- src/proto/vim9execute.pro 2020-10-10 13:53:07.324695567 +0200 *************** *** 1,5 **** --- 1,6 ---- /* vim9execute.c */ void to_string_error(vartype_T vartype); + void funcstack_check_refcount(funcstack_T *funcstack); int call_def_function(ufunc_T *ufunc, int argc_arg, typval_T *argv, partial_T *partial, typval_T *rettv); void ex_disassemble(exarg_T *eap); int tv2bool(typval_T *tv); *** ../vim-8.2.1818/src/structs.h 2020-10-08 21:16:38.643643838 +0200 --- src/structs.h 2020-10-10 13:33:35.539450332 +0200 *************** *** 1869,1876 **** --- 1869,1879 ---- // - arguments // - frame // - local variables + int fs_var_offset; // count of arguments + frame size == offset to + // local variables int fs_refcount; // nr of closures referencing this funcstack + int fs_min_refcount; // nr of closures on this funcstack int fs_copyID; // for garray_T collection } funcstack_T; *** ../vim-8.2.1818/src/eval.c 2020-10-08 21:16:38.643643838 +0200 --- src/eval.c 2020-10-10 14:02:55.015120200 +0200 *************** *** 3984,4004 **** else func_ptr_unref(pt->pt_func); if (pt->pt_funcstack != NULL) { ! // Decrease the reference count for the context of a closure. If down ! // to zero free it and clear the variables on the stack. ! if (--pt->pt_funcstack->fs_refcount == 0) ! { ! garray_T *gap = &pt->pt_funcstack->fs_ga; ! typval_T *stack = gap->ga_data; ! ! for (i = 0; i < gap->ga_len; ++i) ! clear_tv(stack + i); ! ga_clear(gap); ! vim_free(pt->pt_funcstack); ! } ! pt->pt_funcstack = NULL; } vim_free(pt); --- 3984,3995 ---- else func_ptr_unref(pt->pt_func); + // Decrease the reference count for the context of a closure. If down + // to the minimum it may be time to free it. if (pt->pt_funcstack != NULL) { ! --pt->pt_funcstack->fs_refcount; ! funcstack_check_refcount(pt->pt_funcstack); } vim_free(pt); *************** *** 4011,4018 **** void partial_unref(partial_T *pt) { ! if (pt != NULL && --pt->pt_refcount <= 0) ! partial_free(pt); } /* --- 4002,4017 ---- void partial_unref(partial_T *pt) { ! if (pt != NULL) ! { ! if (--pt->pt_refcount <= 0) ! partial_free(pt); ! ! // If the reference count goes down to one, the funcstack may be the ! // only reference and can be freed if no other partials reference it. ! else if (pt->pt_refcount == 1 && pt->pt_funcstack != NULL) ! funcstack_check_refcount(pt->pt_funcstack); ! } } /* *** ../vim-8.2.1818/src/testdir/test_vim9_disassemble.vim 2020-10-08 23:21:17.935678280 +0200 --- src/testdir/test_vim9_disassemble.vim 2020-10-10 14:07:55.922302264 +0200 *************** *** 436,477 **** res) enddef ! " TODO: fix memory leak and enable again ! "def s:CreateRefs() ! " var local = 'a' ! " def Append(arg: string) ! " local ..= arg ! " enddef ! " g:Append = Append ! " def Get(): string ! " return local ! " enddef ! " g:Get = Get ! "enddef ! " ! "def Test_disassemble_closure() ! " CreateRefs() ! " var res = execute('disass g:Append') ! " assert_match('\d\_s*' .. ! " 'local ..= arg\_s*' .. ! " '\d LOADOUTER $0\_s*' .. ! " '\d LOAD arg\[-1\]\_s*' .. ! " '\d CONCAT\_s*' .. ! " '\d STOREOUTER $0\_s*' .. ! " '\d PUSHNR 0\_s*' .. ! " '\d RETURN', ! " res) ! " ! " res = execute('disass g:Get') ! " assert_match('\d\_s*' .. ! " 'return local\_s*' .. ! " '\d LOADOUTER $0\_s*' .. ! " '\d RETURN', ! " res) ! " ! " unlet g:Append ! " unlet g:Get ! "enddef def EchoArg(arg: string): string --- 436,477 ---- res) enddef ! ! def s:CreateRefs() ! var local = 'a' ! def Append(arg: string) ! local ..= arg ! enddef ! g:Append = Append ! def Get(): string ! return local ! enddef ! g:Get = Get ! enddef ! ! def Test_disassemble_closure() ! CreateRefs() ! var res = execute('disass g:Append') ! assert_match('\d\_s*' .. ! 'local ..= arg\_s*' .. ! '\d LOADOUTER $0\_s*' .. ! '\d LOAD arg\[-1\]\_s*' .. ! '\d CONCAT\_s*' .. ! '\d STOREOUTER $0\_s*' .. ! '\d PUSHNR 0\_s*' .. ! '\d RETURN', ! res) ! ! res = execute('disass g:Get') ! assert_match('\d\_s*' .. ! 'return local\_s*' .. ! '\d LOADOUTER $0\_s*' .. ! '\d RETURN', ! res) ! ! unlet g:Append ! unlet g:Get ! enddef def EchoArg(arg: string): string *** ../vim-8.2.1818/src/testdir/test_vim9_func.vim 2020-10-09 22:04:25.210842991 +0200 --- src/testdir/test_vim9_func.vim 2020-10-10 14:08:38.306088907 +0200 *************** *** 1330,1361 **** unlet g:UseVararg enddef ! " TODO: reenable after fixing memory leak ! "def MakeGetAndAppendRefs() ! " var local = 'a' ! " ! " def Append(arg: string) ! " local ..= arg ! " enddef ! " g:Append = Append ! " ! " def Get(): string ! " return local ! " enddef ! " g:Get = Get ! "enddef ! " ! "def Test_closure_append_get() ! " MakeGetAndAppendRefs() ! " g:Get()->assert_equal('a') ! " g:Append('-b') ! " g:Get()->assert_equal('a-b') ! " g:Append('-c') ! " g:Get()->assert_equal('a-b-c') ! " ! " unlet g:Append ! " unlet g:Get ! "enddef def Test_nested_closure() var local = 'text' --- 1330,1360 ---- unlet g:UseVararg enddef ! def MakeGetAndAppendRefs() ! var local = 'a' ! ! def Append(arg: string) ! local ..= arg ! enddef ! g:Append = Append ! ! def Get(): string ! return local ! enddef ! g:Get = Get ! enddef ! ! def Test_closure_append_get() ! MakeGetAndAppendRefs() ! g:Get()->assert_equal('a') ! g:Append('-b') ! g:Get()->assert_equal('a-b') ! g:Append('-c') ! g:Get()->assert_equal('a-b-c') ! ! unlet g:Append ! unlet g:Get ! enddef def Test_nested_closure() var local = 'text' *************** *** 1389,1408 **** CheckScriptSuccess(lines) enddef ! " TODO: reenable after fixing memory leak ! "def Test_nested_closure_used() ! " var lines =<< trim END ! " vim9script ! " def Func() ! " var x = 'hello' ! " var Closure = {-> x} ! " g:Myclosure = {-> Closure()} ! " enddef ! " Func() ! " assert_equal('hello', g:Myclosure()) ! " END ! " CheckScriptSuccess(lines) ! "enddef def Test_nested_closure_fails() var lines =<< trim END --- 1388,1406 ---- CheckScriptSuccess(lines) enddef ! def Test_nested_closure_used() ! var lines =<< trim END ! vim9script ! def Func() ! var x = 'hello' ! var Closure = {-> x} ! g:Myclosure = {-> Closure()} ! enddef ! Func() ! assert_equal('hello', g:Myclosure()) ! END ! CheckScriptSuccess(lines) ! enddef def Test_nested_closure_fails() var lines =<< trim END *** ../vim-8.2.1818/src/version.c 2020-10-09 23:04:43.676144228 +0200 --- src/version.c 2020-10-10 14:09:37.481731088 +0200 *************** *** 752,753 **** --- 752,755 ---- { /* Add new patch number below this line */ + /**/ + 1819, /**/ -- "I can't complain, but sometimes I still do." (Joe Walsh) /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///