Skip to content

fix: Force finalizers to explore the graph of objects #277

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

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jun 28, 2021

First, this patch provides new Close methods on some “top types”
(Store, Module, Instance etc.). The Close methods are
redundant to the runtime finalizers, but they are useful to force to
destruct a specific type manually before the Go GC collect it.

Note that calling Close will remove the runtime finalizer attached
to the object.

Note that runtime finalizers call the Close method if defined on the
current object.

Second, this patch forces the destructors (now the Close methods) to
destruct the children attached to the current object. Let's see it in
details:

  • Module now has two new fields importTypes *importTypes and
    exportTypes *exportTypes. The value is null, except if the
    Imports or Exports methods are called. They will store their
    respective results in their respective fields. The goal of this
    change is twofold:

    1. Avoiding computing the same import and export types multiple
      times each time one of the method is called,

    2. By holding a reference to the private importTypes and
      exportTypes, Module can free them. Before that, the objects
      were orphan and the garbage collector had difficulties to collect
      them.

  • Instance now forces to free its Exports,

  • Exports now force to free all the items of its exports map
    field. Before this patch, only the _inner C pointer was freed, but
    not the map. I have no idea why the map wasn't collected by the Go
    GC.

Running the script written by @prep in
#262 shows that there is
no more memory leaks.

Fix #262.
Fix #269.

@Hywan Hywan added the 🐞 bug Something isn't working label Jun 28, 2021
@Hywan Hywan self-assigned this Jun 28, 2021
This was referenced Jun 28, 2021
First, this patch provides new `Close` methods on some “top types”
(`Store`, `Module`, `Instance` etc.). The `Close` methods are
redundant to the runtime finalizers, but they are useful to force to
destruct a specific type manually before the Go GC collect it.

Note that calling `Close` will remove the runtime finalizer attached
to the object.

Note that runtime finalizers call the `Close` method if defined on the
current object.

Second, this patch forces the destructors (now the `Close` methods) to
destruct the children attached to the current object. Let's see it in
details:

* `Module` now has two new fields `importTypes *importTypes` and
  `exportTypes *exportTypes`. The value is null, except if the
  `Imports` or `Exports` methods are called. They will store their
  respective results in their respective fields. The goal of this
  change is twofold:

  1. Avoiding computing the same import and export types multiple
     times each time one of the method is called,

  2. By holding a reference to the private `importTypes` and
     `exportTypes`, `Module` can free them. Before that, the objects
     were orphan and the garbage collector had difficulties to collect
     them.

* `Instance` now forces to free its `Exports`,

* `Exports` now force to free all the items of its `exports` map
  field. Before this patch, only the `_inner` C pointer was freed, but
  not the map. I have no idea why the map wasn't collected by the Go
  GC.

Running the script written by @prep in
wasmerio#262 shows that there is
no more memory leaks.
@Hywan Hywan force-pushed the fix-instance-leak branch from 173314c to 6c8c75f Compare June 28, 2021 10:25
@Hywan Hywan changed the title [WIP] Fix memory leaks because Go GC doesn't collect as expected fix: Force finalizers to explore the graph of objects Jun 28, 2021
@Hywan Hywan marked this pull request as ready for review June 28, 2021 10:25
@Hywan
Copy link
Contributor Author

Hywan commented Jun 28, 2021

bors try

1 similar comment
@Hywan
Copy link
Contributor Author

Hywan commented Jul 8, 2021

bors try

@bors
Copy link
Contributor

bors bot commented Jul 8, 2021

try

Already running a review

@Hywan Hywan merged commit 72b0251 into wasmerio:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to release an instance? Memory leak
1 participant