iden3 / go-rapidsnark

go-rapidsnark
Apache License 2.0
20 stars 5 forks source link

Suspected memory leak with wazero.NewCircom2WZWitnessCalculator #22

Closed syntrust closed 1 month ago

syntrust commented 2 months ago

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 commented 1 month ago

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?

syntrust commented 1 month ago

@olomix Yes that works for me.