You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Currently, unless Go GC is frequent, the finalizers may not run, and thus the sitter C types will not be freed. Critically, if the bulk of a program's allocations comes from tree sitter, the process can use huge amounts of memory but Go GC won't run as the Go heap isn't growing. Here's a test case that demonstrates this nuance in runtime.SetFinalizer: https://gist.github.com/timruffles/48bfa3bd5a625d80e7c73d0eacfc8335
Suggestion: update the documentation to recommend people Close() treesitter types where possible, to avoid unexpected memory consumption. The finalizer can remain as a backstop, the pattern recommended in this comment from a go maintainer. Other Go common APIs make use of the defer foo.Close() pattern - e.g. files, HTTP responses - so it doesn't appear to me be an undue burden for users. I believe this will remain the more reliable approach even after runtime.AddCleanup is introduced, deprecating runtime.SetFinalizer, as it will have same behaviour as in the test case above.
This isn't something that'll become immediately obvious in one-shot use-cases of treesitter, but becomes more problematic for long-lived use cases, e.g. editors, LSP servers.
In any case, food for thought, and thanks for go-tree-sitter! 🙌
The text was updated successfully, but these errors were encountered:
Currently, unless Go GC is frequent, the finalizers may not run, and thus the sitter C types will not be freed. Critically, if the bulk of a program's allocations comes from tree sitter, the process can use huge amounts of memory but Go GC won't run as the Go heap isn't growing. Here's a test case that demonstrates this nuance in
runtime.SetFinalizer
: https://gist.github.com/timruffles/48bfa3bd5a625d80e7c73d0eacfc8335Suggestion: update the documentation to recommend people
Close()
treesitter types where possible, to avoid unexpected memory consumption. The finalizer can remain as a backstop, the pattern recommended in this comment from a go maintainer. Other Go common APIs make use of thedefer foo.Close()
pattern - e.g. files, HTTP responses - so it doesn't appear to me be an undue burden for users. I believe this will remain the more reliable approach even afterruntime.AddCleanup
is introduced, deprecatingruntime.SetFinalizer
, as it will have same behaviour as in the test case above.This isn't something that'll become immediately obvious in one-shot use-cases of treesitter, but becomes more problematic for long-lived use cases, e.g. editors, LSP servers.
In any case, food for thought, and thanks for go-tree-sitter! 🙌
The text was updated successfully, but these errors were encountered: