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

Remove unnecessary heap allocations and calls to C. #118

Merged

Conversation

koponen-styra
Copy link
Contributor

Instead of allocating the function name and function parameter/return
value holders for each function invocation, pre-allocate them. This is
possible because invocations occur sequentially (in presence of no
threading).

This both reduces GC pressure and removes two cgo calls.

@Hywan Hywan self-assigned this Feb 19, 2020
@Hywan Hywan added 🎉 enhancement New feature or request 📦 component-extension About the Go extension labels Feb 19, 2020
@Hywan Hywan added this to 🌱 In progress in Kanban via automation Feb 19, 2020
Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me, thanks!

Kanban automation moved this from 🌱 In progress to 🔍 Review Feb 19, 2020
@Hywan
Copy link
Contributor

Hywan commented Feb 19, 2020

bors r+

bors bot added a commit that referenced this pull request Feb 19, 2020
118: Remove unnecessary heap allocations and calls to C. r=Hywan a=koponen-styra

Instead of allocating the function name and function parameter/return
value holders for each function invocation, pre-allocate them. This is
possible because invocations occur sequentially (in presence of no
threading).

This both reduces GC pressure and removes two cgo calls.

Co-authored-by: Teemu Koponen <koponen@styra.com>
@bors
Copy link
Contributor

bors bot commented Feb 19, 2020

Build failed

  • Test (macos-latest)
  • Test (ubuntu-latest)

@koponen-styra
Copy link
Contributor Author

Hmm, "just test" passes locally on my ubuntu. I tried to access the details from bors, but it merely says permission denied.

@syrusakbary
Copy link
Member

I think bors failed because some Github permissions. Let's try again

bors r+

bors bot added a commit that referenced this pull request Feb 19, 2020
118: Remove unnecessary heap allocations and calls to C. r=syrusakbary a=koponen-styra

Instead of allocating the function name and function parameter/return
value holders for each function invocation, pre-allocate them. This is
possible because invocations occur sequentially (in presence of no
threading).

This both reduces GC pressure and removes two cgo calls.

Co-authored-by: Teemu Koponen <koponen@styra.com>
@bors
Copy link
Contributor

bors bot commented Feb 19, 2020

Build failed

  • Test (macos-latest)
  • Test (ubuntu-latest)

@syrusakbary
Copy link
Member

syrusakbary commented Feb 19, 2020

The error seem to be in the test bazel build:

Run export GOOS=$(go env GOHOSTOS)
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100   612  100   612    0     0   2571      0 --:--:-- --:--:-- --:--:--  2571

100 6028k  100 6028k    0     0  10.7M      0 --:--:-- --:--:-- --:--:-- 10.7M
2020/02/19 10:09:15 could not resolve the version 'latest' to an actual version number: could not get releases from github.com/bazelbuild/bazel: could not download list of Bazel releases from github.com/bazelbuild: unexpected status code while reading https://api.github.com/repos/bazelbuild/bazel/releases: 403
##[error]Process completed with exit code 1.

This is unrelated to the changes on this PR. Will try to fix it in master upstream and then we can just update this PR with the latest master :)

Note: it seems this is just Github banning Github Actions from getting release information. So we will comment that tests for now

@syrusakbary
Copy link
Member

@koponen-styra if you rebase your PR with the latest master, the issue should be fixed (via 351f772 )

Instead of allocating the function name and function parameter/return
value holders for each function invocation, pre-allocate them. This is
possible because invocations occur sequentially (in presence of no
threading).

This both reduces GC pressure and removes two cgo calls.
@koponen-styra koponen-styra force-pushed the reduce-heap-allocations-and-cgo-calls branch from 03737cf to b297eb6 Compare February 19, 2020 21:54
@syrusakbary
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 20, 2020

Build succeeded

  • Test (macos-latest)
  • Test (ubuntu-latest)

@bors bors bot merged commit d98b4c5 into wasmerio:master Feb 20, 2020
Kanban automation moved this from 🔍 Review to 🎉 Done Feb 20, 2020
@koponen-styra koponen-styra deleted the reduce-heap-allocations-and-cgo-calls branch February 21, 2020 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 component-extension About the Go extension 🎉 enhancement New feature or request
Projects
Kanban
  
🎉 Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants