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

fix(vm): Release VM properly #1116

Merged
merged 5 commits into from
Sep 22, 2023
Merged

Conversation

ajnavarro
Copy link
Contributor

Properly clean slices using make (the internal logic is expecting slices to not be nil).

It closes #1033

  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@netlify
Copy link

netlify bot commented Sep 13, 2023

Deploy Preview for gno-docs failed.

Name Link
🔨 Latest commit 1c27a0f
🔍 Latest deploy log https://app.netlify.com/sites/gno-docs/deploys/6501871923e4370008a0491c

@netlify
Copy link

netlify bot commented Sep 13, 2023

Deploy Preview for gno-docs failed.

Name Link
🔨 Latest commit cee52d1
🔍 Latest deploy log https://app.netlify.com/sites/gno-docs/deploys/650187ca7b8d610008d931da

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Sep 13, 2023
@ajnavarro ajnavarro marked this pull request as ready for review September 13, 2023 10:46
@ajnavarro ajnavarro requested a review from a team September 13, 2023 13:21
@moul moul added this to the 🚀 main.gno.land (required) milestone Sep 16, 2023
@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (e10c0c7) 46.98% compared to head (fa9d905) 46.98%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1116      +/-   ##
==========================================
- Coverage   46.98%   46.98%   -0.01%     
==========================================
  Files         365      365              
  Lines       61159    61154       -5     
==========================================
- Hits        28737    28734       -3     
+ Misses      30062    30061       -1     
+ Partials     2360     2359       -1     
Flag Coverage Δ
gno.land-_test.gnokey 0.00% <ø> (ø)
gno.land-_test.gnoland 88.14% <ø> (ø)
gno.land-_test.pkgs 27.88% <ø> (ø)
gnovm-_test.cmd 45.89% <ø> (ø)
gnovm-_test.gnolang.native 63.09% <ø> (ø)
gnovm-_test.gnolang.other 16.63% <ø> (ø)
gnovm-_test.gnolang.pkg0 17.98% <ø> (ø)
gnovm-_test.gnolang.pkg1 8.21% <ø> (ø)
gnovm-_test.gnolang.pkg2 9.87% <ø> (ø)
gnovm-_test.gnolang.realm 41.68% <ø> (ø)
gnovm-_test.gnolang.stdlibs 53.53% <ø> (ø)
gnovm-_test.pkg 25.94% <100.00%> (-0.02%) ⬇️
tm2-_test.pkg.amino 58.32% <ø> (ø)
tm2-_test.pkg.bft 63.49% <ø> (-0.01%) ⬇️
tm2-_test.pkg.others 59.22% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
gnovm/pkg/gnolang/machine.go 39.60% <100.00%> (-0.23%) ⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Properly clean slices using make (the internal logic is expecting slices
to not be nil).

It closes gnolang#1033

Signed-off-by: Antonio Navarro Perez <[email protected]>
Signed-off-by: Antonio Navarro Perez <[email protected]>
Signed-off-by: Antonio Navarro Perez <[email protected]>
@ajnavarro ajnavarro force-pushed the fix/release-vm-properly branch from 0b09e9b to 3fa88d0 Compare September 19, 2023 10:39
@ajnavarro
Copy link
Contributor Author

@thehowl Applied requested changes and added benchmarks:

PR with makes:
BenchmarkCreateNewMachine-12    	  160599	     13276 ns/op	   45496 B/op	      35 allocs/op

Master branch:
BenchmarkCreateNewMachine-12    	  317624	      7059 ns/op	    3243 B/op	      32 allocs/op

Add changes to the release method:
BenchmarkCreateNewMachine-12    	  318438	      7214 ns/op	    3246 B/op	      32 allocs/op

Add changes to the NewMachineWithOptions method:
BenchmarkCreateNewMachine-12    	  323756	      7193 ns/op	    3240 B/op	      32 allocs/op

@moul moul requested a review from piux2 September 22, 2023 18:46
@moul moul changed the title fix(vm): Release VM properly. fix(vm): Release VM properly Sep 22, 2023
@moul moul merged commit 78466ce into gnolang:master Sep 22, 2023
180 checks passed
gfanton pushed a commit to gfanton/gno that referenced this pull request Nov 9, 2023
Properly clean slices using make (the internal logic is expecting slices
to not be nil).

It closes gnolang#1033

- [X] Added new tests, or not needed, or not feasible
- [X] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [X] Updated the official documentation or not needed
- [X] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [X] Added references to related issues and PRs
- [X] Provided any useful hints for running manual tests
- [X] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).

---------

Signed-off-by: Antonio Navarro Perez <[email protected]>
Co-authored-by: Manfred Touron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

VM is not properly released back to sync pool
4 participants