]> granicus.if.org Git - vim/commitdiff
patch 8.1.1007: using closure may consume a lot of memory v8.1.1007
authorBram Moolenaar <Bram@vim.org>
Thu, 14 Mar 2019 12:43:24 +0000 (13:43 +0100)
committerBram Moolenaar <Bram@vim.org>
Thu, 14 Mar 2019 12:43:24 +0000 (13:43 +0100)
Problem:    Using closure may consume a lot of memory.
Solution:   unreference items that are no longer needed. Add a test. (Ozaki
            Kiichi, closes #3961)

src/testdir/Make_all.mak
src/testdir/test_memory_usage.vim [new file with mode: 0644]
src/userfunc.c
src/version.c

index 774c96b7a415bdb51ff4bc278e2ffc5921dba461..c12a59ea9d419c946e0b760703211281f7d2a1d5 100644 (file)
@@ -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_arglist \
        test_arabic \
+       test_arglist \
        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 (file)
index 0000000..23dcc62
--- /dev/null
@@ -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!\<CR>")
+  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 %\<CR>")
+  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 %\<CR>")
+  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 %\<CR>")
+    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
index 6deb8a9f1f85cb9c55d0c2d46925234d416a82cf..41c660c0c10ce105ea40963a87f2b2d1af32a902 100644 (file)
@@ -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 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. */
-funccall_T *previous_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,43 +586,51 @@ 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];
+       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. */
+       // 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);
+    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 */
+    // 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);
+    // Free all a: variables.
+    vars_clear(&fc->l_avars.dv_hashtab);
 
-    func_ptr_unref(fc->func);
-    vim_free(fc);
+    // 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);
 }
 
 /*
@@ -632,51 +640,75 @@ free_funccal(
     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;
+       int         todo;
+       hashitem_T  *hi;
+       dictitem_T  *di;
 
-       /* "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;
+       free_fc = FALSE;
 
-       /* Make a copy of the a: variables, since we didn't do that above. */
+       // 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);
            }
        }
+    }
+
+    if (may_free_fc && fc->l_varlist.lv_refcount == DO_NOT_FREE_CNT)
+       fc->l_varlist.lv_first = NULL;
+    else
+    {
+       listitem_T *li;
 
-       /* Make a copy of the a:000 items, since we didn't do that above. */
+       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 (free_fc)
+       free_funccal(fc);
+    else
+    {
+       static int made_copy = 0;
 
-       if (++made_copy == 10000)
+       // "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 *fp, int force)
            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 testing)
        {
            fc = *pfc;
            *pfc = fc->caller;
-           free_funccal(fc, TRUE);
+           free_funccal_contents(fc);
            did_free = TRUE;
            did_free_funccal = TRUE;
        }
index 054066ea5fe97ad34e6e7209d6849b2a46890919..fb39cf5910d8be1550e81157c714cd537fb3d7e8 100644 (file)
@@ -779,6 +779,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1007,
 /**/
     1006,
 /**/