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

Suspected memory leak with wazero.NewCircom2WZWitnessCalculator #22

Closed
syntrust opened this issue Aug 7, 2024 · 2 comments · Fixed by #23
Closed

Suspected memory leak with wazero.NewCircom2WZWitnessCalculator #22

syntrust opened this issue Aug 7, 2024 · 2 comments · Fixed by #23
Assignees

Comments

@syntrust
Copy link

syntrust commented Aug 7, 2024

Hello, I have followed the usage in the unit test sample witness/test_wasm_impls/witness_test.go when using wazero.NewCircom2WZWitnessCalculator in my project, and I noticed a continuous increase in memory usage while calculating witnesses using CalculateWTNSBin. I am not sure if it is a memory leak but, when I keep the unit test case running with pprof, it seems that the memory usage also increases.
Please correct me if I am wrong, but is the Close method supposed to solve this issue? If so, when and how should it be used? Any feedback would be appreciated.

      flat  flat%   sum%        cum   cum%
  967.83MB 57.61% 57.61%   967.83MB 57.61%  github.com/tetratelabs/wazero/internal/wasm/binary.decodeCode
  412.66MB 24.56% 82.17%   413.16MB 24.59%  github.com/tetratelabs/wazero/internal/wasm/binary.decodeDataSegment
   93.30MB  5.55% 87.73%   125.81MB  7.49%  github.com/tetratelabs/wazero/internal/wasm/binary.decodeLocalNames
   60.25MB  3.59% 91.31%    74.75MB  4.45%  github.com/tetratelabs/wazero/internal/wasm.(*Module).BuildFunctionDefinitions
      36MB  2.14% 93.46%       36MB  2.14%  github.com/tetratelabs/wazero/internal/wasm/binary.decodeUTF8
   28.69MB  1.71% 95.16%    40.92MB  2.44%  github.com/tetratelabs/wazero/internal/engine/compiler.(*engine).CompileModule
   25.99MB  1.55% 96.71%   993.82MB 59.16%  github.com/tetratelabs/wazero/internal/wasm/binary.decodeCodeSection
   15.87MB  0.94% 97.66%   429.02MB 25.54%  github.com/tetratelabs/wazero/internal/wasm/binary.decodeDataSection
    8.50MB  0.51% 98.16%     8.50MB  0.51%  github.com/tetratelabs/wazero/internal/wasm.paramNames (inline)
    7.64MB  0.45% 98.62%    11.14MB  0.66%  github.com/tetratelabs/wazero/internal/wasm/binary.decodeFunctionNames
olomix added a commit that referenced this issue Sep 10, 2024
… a method Close to Calculator interface. It calls the Close method on the underlying CalculatorImpl if one is supports optional io.Closer interface.
olomix added a commit that referenced this issue Sep 10, 2024
… a method Close to Calculator interface. It calls the Close method on the underlying CalculatorImpl if one is supports optional io.Closer interface.
@olomix
Copy link
Collaborator

olomix commented Sep 10, 2024

Actually it was not required to call the Close method on wazero components as this method only clears caches. All compiled wasm artifacts should be cleaned up by GC. But you have a good point to add this method as other calculator implementations could potentially require the Close method call to free resources. It could be worked around by instantiating it and call the Close by yourself, but for convenience, I have added the method to Calculator interface and now it calls Close method on the underlying CalculatorImpl if it is has it. It is still optional for CalculatorImpl to implement the Close method. In this case Calculator.Close would do nothing.

I can confirm the memory leak. It turns out resources was not freed acquired by wasm compilation process. It seems was a bug in the wazero module. After upgrading it to the latest version it looks like everything works as expected. @syntrust could you check please that PR #23 works for you?

olomix added a commit that referenced this issue Sep 13, 2024
…erface (#23)

* Upgrade wazero module to v1.8.0, that fixes #22 memory leak. Also add a method Close to Calculator interface. It calls the Close method on the underlying CalculatorImpl if one is supports optional io.Closer interface.

* Call the Close method on witness calculator

* Upgrade golangci-lint version

* Upgrade Go version we test witnesscalc from 1.21 to 1.23
@syntrust
Copy link
Author

@olomix Yes that works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants