Skip to content

fix: Use Go memory allocator for arrow #810

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.

Sign up for GitHub

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

Jump to bottom
Merged
kodiakhq merged 2 commits into from
Apr 20, 2023
Merged

fix: Use Go memory allocator for arrow #810

kodiakhq merged 2 commits into from
Apr 20, 2023

Conversation

@yevgenypats
Copy link
Contributor

@yevgenypats yevgenypats commented Apr 19, 2023

Following this short discussion - apache/arrow#35232

It seems we don't really need to use Retain/Release outside the arrow library. We can always bring this back in the future if we would like to experiment if this brings better memory performance then the default go allocator.

@github-actions github-actions bot added the fix label Apr 19, 2023
@github-actions
Copy link

github-actions bot commented Apr 19, 2023 •
edited
Loading

⏱️ Benchmark results

  • DefaultConcurrencyDFS-2 resources/s: 11,011
  • DefaultConcurrencyRoundRobin-2 resources/s: 11,003
  • Glob-2 ns/op: 183.1
  • TablesWithChildrenDFS-2 resources/s: 27,341
  • TablesWithChildrenRoundRobin-2 resources/s: 29,184
  • TablesWithRateLimitingDFS-2 resources/s: 28.52
  • TablesWithRateLimitingRoundRobin-2 resources/s: 809.6
  • BufferedScanner-2 ns/op: 9.292
  • LogReader-2 ns/op: 30.57

@codecov
Copy link

codecov bot commented Apr 19, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: -0.09 ⚠️

Comparison is base (2dfeb02) 46.71% compared to head (0463c5e) 46.63%.

❗ Current head 0463c5e differs from pull request most recent head f812324. Consider uploading reports for the commit f812324 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
- Coverage   46.71%   46.63%   -0.09%     
==========================================
  Files          76       76              
  Lines        7832     7820      -12     
==========================================
- Hits         3659     3647      -12     
  Misses       3682     3682              
  Partials      491      491
Impacted Files Coverage Δ
internal/memdb/memdb.go 83.66% <ø> (-1.19%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yevgenypats yevgenypats mentioned this pull request Apr 19, 2023
Merged
@candiduslynx
Copy link
Contributor

candiduslynx commented Apr 19, 2023

So, we should update the plugin code as well? (Stop using release with this update?)

@yevgenypats
Copy link
Contributor Author

yevgenypats commented Apr 19, 2023

So, we should update the plugin code as well? (Stop using release with this update?)

Yes.

@kodiakhq kodiakhq bot merged commit b54e5e1 into main Apr 20, 2023
@kodiakhq kodiakhq bot deleted the feat/use_go_gc branch April 20, 2023 05:33
@cq-bot cq-bot mentioned this pull request Apr 20, 2023
Merged
hermanschaaf
hermanschaaf reviewed Apr 20, 2023
View reviewed changes
Copy link
Member

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

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

Nice 💪

kodiakhq bot pushed a commit that referenced this pull request Apr 20, 2023
@cq-bot
chore(main): Release v2.3.7 (#813)
a61f34e 
🤖 I have created a release *beep* *boop*
---


## [2.3.7](v2.3.6...v2.3.7) (2023-04-20)


### Bug Fixes

* Use Go memory allocator for arrow ([#810](#810)) ([b54e5e1](b54e5e1))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@erezrokah erezrokah mentioned this pull request Apr 20, 2023
Merged
@erezrokah
Copy link
Member

erezrokah commented Apr 20, 2023

Follow up question, do we need the memory checks we have in the some of the tests

plugin-sdk/types/inet_test.go

Line 21 in a61f34e

mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
if we use the Go allocator?

This was referenced Apr 20, 2023
Merged
Merged
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Apr 20, 2023
@erezrokah
fix: Update to SDK v2.3.7, remove release calls (#10211)
dc9e0d5 

#### Summary

Follow up to cloudquery/plugin-sdk#810

<!--
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Apr 20, 2023
@erezrokah
fix: Update to SDK v2.3.7, remove release calls (#10212)
ac3d503 

#### Summary

Follow up to cloudquery/plugin-sdk#810

<!--
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Apr 20, 2023
@erezrokah
fix: Update to SDK v2.3.7, remove release calls (#10209)
5442837 

#### Summary

Follow up to cloudquery/plugin-sdk#810

<!--
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hermanschaaf hermanschaaf hermanschaaf left review comments

@erezrokah erezrokah erezrokah approved these changes

Assignees

No one assigned

Labels

automerge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yevgenypats @candiduslynx @erezrokah @hermanschaaf