Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify function-call cleanup #3961

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
3 participants
@ichizok
Copy link

commented Feb 14, 2019

Ref: #3835

Proposal

At the end of function call, if "funccall" object and its "l:" vars are not referred by anyone,
Vim may be able to free "a:", "a:000" vars, so and "funccall".

Therefore, this patch changes:

  • In cleanup_function_call(), check the references of "l:", "a:", "a:000" separately and free each of them if can (or retain)

By doing this, in the case of #3835 "funccall" object can be freed just after its function ends,
so the memory usage doesn't gain almost and then needs no GC here.

Confirmation

On Ubuntu 18.04.

test.vim

func s:f(...)
  let l:x = a:000
endfunc
for _ in range(100000)
  call s:f(0)
endfor
$ vim --clean test.vim
:so %

monitoring memory usage:

$ while :; do grep '^VmRSS' /proc/$(pgrep vim)/status; sleep 1; done

8.1.0918:

VmRSS:      8712 kB
VmRSS:      8712 kB
VmRSS:      8712 kB
VmRSS:    225716 kB    # do ":so %"
VmRSS:    225716 kB
VmRSS:    225716 kB
VmRSS:    225716 kB    # though GC was done, RSS doesn't decrease immediately because of cached
...

patched:

VmRSS:      8740 kB
VmRSS:      8740 kB
VmRSS:      8740 kB
VmRSS:     13224 kB    # do ":so %"
VmRSS:     13224 kB
VmRSS:     13224 kB
VmRSS:     13224 kB    # no change also after this
...
@codecov-io

This comment has been minimized.

Copy link

commented Feb 14, 2019

Codecov Report

Merging #3961 into master will decrease coverage by 0.05%.
The diff coverage is 97.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3961      +/-   ##
==========================================
- Coverage   79.14%   79.09%   -0.06%     
==========================================
  Files         105      105              
  Lines      141127   141247     +120     
==========================================
+ Hits       111701   111715      +14     
- Misses      29426    29532     +106
Impacted Files Coverage Δ
src/userfunc.c 87.99% <97.43%> (+0.07%) ⬆️
src/version.c 58.2% <0%> (-28.08%) ⬇️
src/if_xcmdsrv.c 83.48% <0%> (-0.54%) ⬇️
src/ex_cmds.c 81.63% <0%> (-0.25%) ⬇️
src/channel.c 83.14% <0%> (-0.08%) ⬇️
src/window.c 83.53% <0%> (-0.07%) ⬇️
src/sign.c 93.51% <0%> (+0.12%) ⬆️
src/gui_gtk_x11.c 48.57% <0%> (+0.29%) ⬆️
src/gui.c 58.51% <0%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e86ecbd...6796e91. Read the comment docs.

@brammool

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@ichizok

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

The change to .travis.yml is needed? Or was this just for testing?

Though this is not related to the subject of this pull-req, I felt it is too small to make a new pull-req for only this change so I included to this patch.

This change means:
Bash executes "while read" inside subprocess so lose variables at the end of loop, thus "log" variable is always empty outside the loop and this block exits with 0.
By finishing the inside of "do ~ done" by "false" it makes whole this block exit with 1.

I don't like renaming vars_clear_ext() to vars_clear_ht().
vars_clear also takes a hashtab_T argument, the difference is that the
_ext function also takes a flag, thus it's "extended".

I understand. I updated the patch and reverted renaming.

@ichizok ichizok force-pushed the ichizok:fix/funccall_cleanup branch 2 times, most recently from 61e4114 to 5a7d620 Feb 18, 2019

@ichizok

This comment has been minimized.

Copy link
Author

commented Feb 21, 2019

I updated the patch: I removed the change for .travis.yml since because the additional changes (and I will create new pull-req for it).

@ichizok ichizok force-pushed the ichizok:fix/funccall_cleanup branch from 5a7d620 to 6796e91 Feb 21, 2019

@brammool brammool closed this in 209b8e3 Mar 14, 2019

@ichizok ichizok deleted the ichizok:fix/funccall_cleanup branch Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.