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

[RFC] Speed up garbage collection (Issue 1687). #1761

Merged
merged 1 commit into from Jan 10, 2015

Conversation

Projects
None yet
6 participants
@oni-link
Contributor

oni-link commented Dec 31, 2014

Speed up garbage collection. (Issue #1687)

For garbage collection all lists are kept in first_list, a list of all
lists.

free_unref_items() searches through first_list and removes unreferenced
lists from it (by calling list_free(..., FALSE)). But after a list was
removed, the search continues from the beginning of first_list (not sure
how many lists were really removed and where to continue in first_list).

This is not necessary anymore since vim-patch 7.0.135 , because a call to
list_free(...,FALSE) makes sure, that no other lists (and dictionaries)
are freed. So we always know, that the next list in first_list is
still valid (allocated or NULL) and can be used to continue the search.

Likewise for dictionaries.

Original patch by Ariya Mizutani
https://groups.google.com/forum/#!searchin/vim_dev/GC/vim_dev/DBYOdHQWvqY/1WH04_dwETIJ

Speed up garbage collection.
For garbage collection all lists are kept in first_list, a list of all
lists.

free_unref_items() searches through first_list and removes unreferenced
lists from it (by calling list_free(..., FALSE)). But after a list was
removed, the search continues from the beginning of first_list (not sure
how many lists were really removed and where to continue in first_list).

This is not necessary anymore since vim-patch 7.0.135, because a call to
list_free(...,FALSE) makes sure, that no other lists (and dictionaries)
are freed. So we always know, that the next list in first_list is
still valid (allocated or NULL) and can be used to continue the search.

Likewise for dictionaries.

Original patch by Ariya Mizutani
https://groups.google.com/forum/#!searchin/vim_dev/GC/vim_dev/DBYOdHQWvqY/1WH04_dwETIJ

@marvim marvim added the RFC label Dec 31, 2014

@Shougo

This comment has been minimized.

Show comment
Hide comment
@Shougo

Shougo Jan 6, 2015

Contributor

👍

Contributor

Shougo commented Jan 6, 2015

👍

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 7, 2015

Member

Very nice. What are the before/after results for the test case in the vim_dev link?

Member

justinmk commented Jan 7, 2015

Very nice. What are the before/after results for the test case in the vim_dev link?

@demonking

This comment has been minimized.

Show comment
Hide comment
@demonking

demonking Jan 7, 2015

@justinmk what do you mean by vim_dev?

In the Link the Result was the following:

Without this patch, it take 100 seconds to finish GC. And if set 100000 instead of 50000, vim often hangs. With this patch, it take just 0.133967 seconds.

demonking commented Jan 7, 2015

@justinmk what do you mean by vim_dev?

In the Link the Result was the following:

Without this patch, it take 100 seconds to finish GC. And if set 100000 instead of 50000, vim often hangs. With this patch, it take just 0.133967 seconds.

@oni-link

This comment has been minimized.

Show comment
Hide comment
@oni-link

oni-link Jan 7, 2015

Contributor

When I run the following script in nvim/vim the times shown are similar (0.074/0.070). But vim (or unpatched nvim) blocks for me >28s after showing the output time.

let s:hoge = {}
let s:fuga = {}
let s:arr = []

function! Prepare() abort " {{{
  let s:hoge.a1 = map(range(1, 50000), '{}')
  let s:hoge.a2 = map(range(1, 50000), '{}')
  let s:hoge.a3 = map(range(1, 50000), '{}')
  let s:hoge.fuga = s:fuga
  let s:fuga.hoge = s:hoge
endfunction " }}}

function! Release() abort " {{{
  let s:arr = s:hoge.a2
  unlet s:hoge
  unlet s:fuga
endfunction " }}}

let start = reltime()
call Prepare()
call Release()
call garbagecollect()
echomsg string(reltimestr(reltime(start)))
Contributor

oni-link commented Jan 7, 2015

When I run the following script in nvim/vim the times shown are similar (0.074/0.070). But vim (or unpatched nvim) blocks for me >28s after showing the output time.

let s:hoge = {}
let s:fuga = {}
let s:arr = []

function! Prepare() abort " {{{
  let s:hoge.a1 = map(range(1, 50000), '{}')
  let s:hoge.a2 = map(range(1, 50000), '{}')
  let s:hoge.a3 = map(range(1, 50000), '{}')
  let s:hoge.fuga = s:fuga
  let s:fuga.hoge = s:hoge
endfunction " }}}

function! Release() abort " {{{
  let s:arr = s:hoge.a2
  unlet s:hoge
  unlet s:fuga
endfunction " }}}

let start = reltime()
call Prepare()
call Release()
call garbagecollect()
echomsg string(reltimestr(reltime(start)))
@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Jan 7, 2015

Contributor

Perhaps this would help in the locking scenario in issue #1776 (when there is no corruption).

Contributor

fmoralesc commented Jan 7, 2015

Perhaps this would help in the locking scenario in issue #1776 (when there is no corruption).

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 7, 2015

Member

Tried this patch, but it didn't appear to change the behavior of #1176.

Member

justinmk commented Jan 7, 2015

Tried this patch, but it didn't appear to change the behavior of #1176.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Jan 7, 2015

Contributor

Just tried it, and no, it doesn't seem to help with locking in #1776.

I tried @oni-link script, and I can confirm his report.

Contributor

fmoralesc commented Jan 7, 2015

Just tried it, and no, it doesn't seem to help with locking in #1776.

I tried @oni-link script, and I can confirm his report.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 10, 2015

Member

This is not necessary anymore since vim-patch 7.0.135, because a call to list_free(...,FALSE) makes sure, that no other lists (and dictionaries) are freed. So we always know, that the next list in first_list is still valid (allocated or NULL) and can be used to continue the search.

It appears that 7.0.135 was focused on eliminating the DEL_REFCOUNT magic global and simply missed the optimization opportunity hiding in plain sight. Nice find @oni-link

Member

justinmk commented Jan 10, 2015

This is not necessary anymore since vim-patch 7.0.135, because a call to list_free(...,FALSE) makes sure, that no other lists (and dictionaries) are freed. So we always know, that the next list in first_list is still valid (allocated or NULL) and can be used to continue the search.

It appears that 7.0.135 was focused on eliminating the DEL_REFCOUNT magic global and simply missed the optimization opportunity hiding in plain sight. Nice find @oni-link

justinmk added a commit that referenced this pull request Jan 10, 2015

Merge pull request #1761 from oni-link/speed.up.gc
Speed up garbage collection (Issue 1687).

@justinmk justinmk merged commit a684cc1 into neovim:master Jan 10, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@justinmk justinmk removed the RFC label Jan 10, 2015

@Shougo

This comment has been minimized.

Show comment
Hide comment
@Shougo

Shougo Jan 13, 2015

Contributor

👍

Contributor

Shougo commented Jan 13, 2015

👍

@Shougo

This comment has been minimized.

Show comment
Hide comment
@Shougo

Shougo Feb 3, 2015

Contributor

It is included in Vim 7.4.615.

http://ftp.vim.org/vim/patches/7.4/7.4.615

Contributor

Shougo commented Feb 3, 2015

It is included in Vim 7.4.615.

http://ftp.vim.org/vim/patches/7.4/7.4.615

@ghost ghost referenced this pull request Feb 21, 2015

Merged

Upcoming Newsletter #82

22 of 22 tasks complete

justinmk added a commit that referenced this pull request Mar 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment