# HG changeset patch # User Bram Moolenaar # Date 1552567506 -3600 # Node ID 879829e44091d16b909d772cc456a2f3317cff05 # Parent 7834d7d14b488202ae12486e99ba6c78b2846688 patch 8.1.1007: using closure may consume a lot of memory commit https://github.com/vim/vim/commit/209b8e3e3bf7a4a3d102134124120f6c7f57d560 Author: Bram Moolenaar Date: Thu Mar 14 13:43:24 2019 +0100 patch 8.1.1007: using closure may consume a lot of memory Problem: Using closure may consume a lot of memory. Solution: unreference items that are no longer needed. Add a test. (Ozaki Kiichi, closes #3961) diff --git a/src/testdir/Make_all.mak b/src/testdir/Make_all.mak --- a/src/testdir/Make_all.mak +++ b/src/testdir/Make_all.mak @@ -63,8 +63,8 @@ SCRIPTS_GUI = # Individual tests, including the ones part of test_alot. # Please keep sorted up to test_alot. NEW_TESTS = \ + test_arabic \ test_arglist \ - test_arabic \ test_assert \ test_assign \ test_autochdir \ @@ -108,11 +108,11 @@ NEW_TESTS = \ test_ex_equal \ test_ex_undo \ test_ex_z \ - test_exit \ test_exec_while_if \ test_execute_func \ test_exists \ test_exists_autocmd \ + test_exit \ test_expand \ test_expand_dllpath \ test_expand_func \ @@ -179,6 +179,7 @@ NEW_TESTS = \ test_match \ test_matchadd_conceal \ test_matchadd_conceal_utf8 \ + test_memory_usage \ test_menu \ test_messages \ test_mksession \ @@ -355,6 +356,7 @@ NEW_TESTS_RES = \ test_maparg.res \ test_marks.res \ test_matchadd_conceal.res \ + test_memory_usage.res \ test_mksession.res \ test_nested_function.res \ test_netbeans.res \ diff --git a/src/testdir/test_memory_usage.vim b/src/testdir/test_memory_usage.vim new file mode 100644 --- /dev/null +++ b/src/testdir/test_memory_usage.vim @@ -0,0 +1,144 @@ +" Tests for memory usage. + +if !has('terminal') || has('gui_running') || $ASAN_OPTIONS !=# '' + " Skip tests on Travis CI ASAN build because it's difficult to estimate + " memory usage. + finish +endif + +source shared.vim + +func s:pick_nr(str) abort + return substitute(a:str, '[^0-9]', '', 'g') * 1 +endfunc + +if has('win32') + if !executable('wmic') + finish + endif + func s:memory_usage(pid) abort + let cmd = printf('wmic process where processid=%d get WorkingSetSize', a:pid) + return s:pick_nr(system(cmd)) / 1024 + endfunc +elseif has('unix') + if !executable('ps') + finish + endif + func s:memory_usage(pid) abort + return s:pick_nr(system('ps -o rss= -p ' . a:pid)) + endfunc +else + finish +endif + +" Wait for memory usage to level off. +func s:monitor_memory_usage(pid) abort + let proc = {} + let proc.pid = a:pid + let proc.hist = [] + let proc.min = 0 + let proc.max = 0 + + func proc.op() abort + " Check the last 200ms. + let val = s:memory_usage(self.pid) + if self.min > val + let self.min = val + elseif self.max < val + let self.max = val + endif + call add(self.hist, val) + if len(self.hist) < 20 + return 0 + endif + let sample = remove(self.hist, 0) + return len(uniq([sample] + self.hist)) == 1 + endfunc + + call WaitFor({-> proc.op()}, 10000) + return {'last': get(proc.hist, -1), 'min': proc.min, 'max': proc.max} +endfunc + +let s:term_vim = {} + +func s:term_vim.start(...) abort + let self.buf = term_start([GetVimProg()] + a:000) + let self.job = term_getjob(self.buf) + call WaitFor({-> job_status(self.job) ==# 'run'}) + let self.pid = job_info(self.job).process +endfunc + +func s:term_vim.stop() abort + call term_sendkeys(self.buf, ":qall!\") + call WaitFor({-> job_status(self.job) ==# 'dead'}) + exe self.buf . 'bwipe!' +endfunc + +func s:vim_new() abort + return copy(s:term_vim) +endfunc + +func Test_memory_func_capture_vargs() + " Case: if a local variable captures a:000, funccall object will be free + " just after it finishes. + let testfile = 'Xtest.vim' + call writefile([ + \ 'func s:f(...)', + \ ' let x = a:000', + \ 'endfunc', + \ 'for _ in range(10000)', + \ ' call s:f(0)', + \ 'endfor', + \ ], testfile) + + let vim = s:vim_new() + call vim.start('--clean', '-c', 'set noswapfile', testfile) + let before = s:monitor_memory_usage(vim.pid).last + + call term_sendkeys(vim.buf, ":so %\") + call WaitFor({-> term_getcursor(vim.buf)[0] == 1}) + let after = s:monitor_memory_usage(vim.pid) + + " Estimate the limit of max usage as 2x initial usage. + call assert_inrange(before, 2 * before, after.max) + " In this case, garbase collecting is not needed. + call assert_equal(after.last, after.max) + + call vim.stop() + call delete(testfile) +endfunc + +func Test_memory_func_capture_lvars() + " Case: if a local variable captures l: dict, funccall object will not be + " free until garbage collector runs, but after that memory usage doesn't + " increase so much even when rerun Xtest.vim since system memory caches. + let testfile = 'Xtest.vim' + call writefile([ + \ 'func s:f()', + \ ' let x = l:', + \ 'endfunc', + \ 'for _ in range(10000)', + \ ' call s:f()', + \ 'endfor', + \ ], testfile) + + let vim = s:vim_new() + call vim.start('--clean', '-c', 'set noswapfile', testfile) + let before = s:monitor_memory_usage(vim.pid).last + + call term_sendkeys(vim.buf, ":so %\") + call WaitFor({-> term_getcursor(vim.buf)[0] == 1}) + let after = s:monitor_memory_usage(vim.pid) + + " Rerun Xtest.vim. + for _ in range(3) + call term_sendkeys(vim.buf, ":so %\") + call WaitFor({-> term_getcursor(vim.buf)[0] == 1}) + let last = s:monitor_memory_usage(vim.pid).last + endfor + + call assert_inrange(before, after.max + (after.last - before), last) + + call vim.stop() + call delete(testfile) +endfunc diff --git a/src/userfunc.c b/src/userfunc.c --- a/src/userfunc.c +++ b/src/userfunc.c @@ -39,12 +39,12 @@ static hashtab_T func_hashtab; /* Used by get_func_tv() */ static garray_T funcargs = GA_EMPTY; -/* pointer to funccal for currently active function */ -funccall_T *current_funccal = NULL; - -/* Pointer to list of previously used funccal, still around because some - * item in it is still being used. */ -funccall_T *previous_funccal = NULL; +// pointer to funccal for currently active function +static funccall_T *current_funccal = NULL; + +// Pointer to list of previously used funccal, still around because some +// item in it is still being used. +static funccall_T *previous_funccal = NULL; static char *e_funcexts = N_("E122: Function %s already exists, add ! to replace it"); static char *e_funcdict = N_("E717: Dictionary entry already exists"); @@ -586,97 +586,129 @@ add_nr_var( } /* - * Free "fc" and what it contains. + * Free "fc". */ - static void -free_funccal( - funccall_T *fc, - int free_val) /* a: vars were allocated */ + static void +free_funccal(funccall_T *fc) { - listitem_T *li; - int i; + int i; for (i = 0; i < fc->fc_funcs.ga_len; ++i) { - ufunc_T *fp = ((ufunc_T **)(fc->fc_funcs.ga_data))[i]; - - /* When garbage collecting a funccall_T may be freed before the - * function that references it, clear its uf_scoped field. - * The function may have been redefined and point to another - * funccall_T, don't clear it then. */ + ufunc_T *fp = ((ufunc_T **)(fc->fc_funcs.ga_data))[i]; + + // When garbage collecting a funccall_T may be freed before the + // function that references it, clear its uf_scoped field. + // The function may have been redefined and point to another + // funccall_T, don't clear it then. if (fp != NULL && fp->uf_scoped == fc) fp->uf_scoped = NULL; } ga_clear(&fc->fc_funcs); - /* The a: variables typevals may not have been allocated, only free the - * allocated variables. */ - vars_clear_ext(&fc->l_avars.dv_hashtab, free_val); - - /* free all l: variables */ - vars_clear(&fc->l_vars.dv_hashtab); - - /* Free the a:000 variables if they were allocated. */ - if (free_val) - for (li = fc->l_varlist.lv_first; li != NULL; li = li->li_next) - clear_tv(&li->li_tv); - func_ptr_unref(fc->func); vim_free(fc); } /* + * Free "fc" and what it contains. + * Can be called only when "fc" is kept beyond the period of it called, + * i.e. after cleanup_function_call(fc). + */ + static void +free_funccal_contents(funccall_T *fc) +{ + listitem_T *li; + + // Free all l: variables. + vars_clear(&fc->l_vars.dv_hashtab); + + // Free all a: variables. + vars_clear(&fc->l_avars.dv_hashtab); + + // Free the a:000 variables. + for (li = fc->l_varlist.lv_first; li != NULL; li = li->li_next) + clear_tv(&li->li_tv); + + free_funccal(fc); +} + +/* * Handle the last part of returning from a function: free the local hashtable. * Unless it is still in use by a closure. */ static void cleanup_function_call(funccall_T *fc) { + int may_free_fc = fc->fc_refcount <= 0; + int free_fc = TRUE; + current_funccal = fc->caller; - /* If the a:000 list and the l: and a: dicts are not referenced and there - * is no closure using it, we can free the funccall_T and what's in it. */ - if (fc->l_varlist.lv_refcount == DO_NOT_FREE_CNT - && fc->l_vars.dv_refcount == DO_NOT_FREE_CNT - && fc->l_avars.dv_refcount == DO_NOT_FREE_CNT - && fc->fc_refcount <= 0) - { - free_funccal(fc, FALSE); - } + // Free all l: variables if not referred. + if (may_free_fc && fc->l_vars.dv_refcount == DO_NOT_FREE_CNT) + vars_clear(&fc->l_vars.dv_hashtab); + else + free_fc = FALSE; + + // If the a:000 list and the l: and a: dicts are not referenced and + // there is no closure using it, we can free the funccall_T and what's + // in it. + if (may_free_fc && fc->l_avars.dv_refcount == DO_NOT_FREE_CNT) + vars_clear_ext(&fc->l_avars.dv_hashtab, FALSE); else { - hashitem_T *hi; - listitem_T *li; - int todo; - dictitem_T *v; - static int made_copy = 0; - - /* "fc" is still in use. This can happen when returning "a:000", - * assigning "l:" to a global variable or defining a closure. - * Link "fc" in the list for garbage collection later. */ - fc->caller = previous_funccal; - previous_funccal = fc; - - /* Make a copy of the a: variables, since we didn't do that above. */ + int todo; + hashitem_T *hi; + dictitem_T *di; + + free_fc = FALSE; + + // Make a copy of the a: variables, since we didn't do that above. todo = (int)fc->l_avars.dv_hashtab.ht_used; for (hi = fc->l_avars.dv_hashtab.ht_array; todo > 0; ++hi) { if (!HASHITEM_EMPTY(hi)) { --todo; - v = HI2DI(hi); - copy_tv(&v->di_tv, &v->di_tv); + di = HI2DI(hi); + copy_tv(&di->di_tv, &di->di_tv); } } - - /* Make a copy of the a:000 items, since we didn't do that above. */ + } + + if (may_free_fc && fc->l_varlist.lv_refcount == DO_NOT_FREE_CNT) + fc->l_varlist.lv_first = NULL; + else + { + listitem_T *li; + + free_fc = FALSE; + + // Make a copy of the a:000 items, since we didn't do that above. for (li = fc->l_varlist.lv_first; li != NULL; li = li->li_next) copy_tv(&li->li_tv, &li->li_tv); - - if (++made_copy == 10000) + } + + if (free_fc) + free_funccal(fc); + else + { + static int made_copy = 0; + + // "fc" is still in use. This can happen when returning "a:000", + // assigning "l:" to a global variable or defining a closure. + // Link "fc" in the list for garbage collection later. + fc->caller = previous_funccal; + previous_funccal = fc; + + if (want_garbage_collect) + // If garbage collector is ready, clear count. + made_copy = 0; + else if (++made_copy >= (int)((4096 * 1024) / sizeof(*fc))) { - // We have made a lot of copies. This can happen when - // repetitively calling a function that creates a reference to + // We have made a lot of copies, worth 4 Mbyte. This can happen + // when repetitively calling a function that creates a reference to // itself somehow. Call the garbage collector soon to avoid using // too much memory. made_copy = 0; @@ -731,7 +763,7 @@ call_user_func( line_breakcheck(); /* check for CTRL-C hit */ - fc = (funccall_T *)alloc(sizeof(funccall_T)); + fc = (funccall_T *)alloc_clear(sizeof(funccall_T)); if (fc == NULL) return; fc->caller = current_funccal; @@ -832,16 +864,15 @@ call_user_func( { v = &fc->fixvar[fixvar_idx++].var; v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX; + STRCPY(v->di_key, name); } else { - v = (dictitem_T *)alloc((unsigned)(sizeof(dictitem_T) - + STRLEN(name))); + v = dictitem_alloc(name); if (v == NULL) break; - v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX | DI_FLAGS_ALLOC; + v->di_flags |= DI_FLAGS_RO | DI_FLAGS_FIX; } - STRCPY(v->di_key, name); /* Note: the values are copied directly to avoid alloc/free. * "argvars" must have VAR_FIXED for v_lock. */ @@ -860,9 +891,11 @@ call_user_func( if (ai >= 0 && ai < MAX_FUNC_ARGS) { - list_append(&fc->l_varlist, &fc->l_listitems[ai]); - fc->l_listitems[ai].li_tv = argvars[i]; - fc->l_listitems[ai].li_tv.v_lock = VAR_FIXED; + listitem_T *li = &fc->l_listitems[ai]; + + li->li_tv = argvars[i]; + li->li_tv.v_lock = VAR_FIXED; + list_append(&fc->l_varlist, li); } } @@ -1088,7 +1121,7 @@ funccal_unref(funccall_T *fc, ufunc_T *f if (fc == *pfc) { *pfc = fc->caller; - free_funccal(fc, TRUE); + free_funccal_contents(fc); return; } } @@ -3646,7 +3679,7 @@ free_unref_funccal(int copyID, int testi { fc = *pfc; *pfc = fc->caller; - free_funccal(fc, TRUE); + free_funccal_contents(fc); did_free = TRUE; did_free_funccal = TRUE; } diff --git a/src/version.c b/src/version.c --- a/src/version.c +++ b/src/version.c @@ -780,6 +780,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ /**/ + 1007, +/**/ 1006, /**/ 1005,