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.

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

Sdk memory optimizations #2622

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Sdk memory optimizations #2622

merged 3 commits into from
Nov 3, 2023

Conversation

smallhive
Copy link
Contributor

No description provided.

roman-khimov
roman-khimov reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Looks OK for the node (IR is also somewhat questionable, but doesn't hurt there) while CLI can be left as is.

cmd/neofs-cli/internal/client/sdk.go Outdated Show resolved Hide resolved
pkg/local_object_storage/writecache/status.go Outdated
"github.com/nspcc-dev/neofs-node/pkg/local_object_storage/blobstor/common"
oid "github.com/nspcc-dev/neofs-sdk-go/object/id"
"github.com/pkg/errors"
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it went unnoticed, I've also encountered this in https://github.com/nspcc-dev/neofs-node/pull/2620/commits

Copy link
Member

Choose a reason for hiding this comment

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

@roman-khimov, @smallhive, @cthulhu-rider, how about https://golangci-lint.run/usage/linters/#gomodguard?

Copy link
Member

Choose a reason for hiding this comment

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

Can't say this is critical, but a linter can be added. Not in this PR anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am okay with it, but it looks overkill

Copy link
Member

@carpawell carpawell Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we have made a mistake once so let it be a PR at least: #2628

@smallhive smallhive force-pushed the sdk-memory-optimizations branch 3 times, most recently from d85ae9f to 6c6b2fe Compare October 23, 2023 06:31
@smallhive smallhive marked this pull request as ready for review October 23, 2023 06:31
carpawell
carpawell reviewed Oct 23, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
carpawell
carpawell reviewed Oct 23, 2023
View reviewed changes
cmd/neofs-node/config.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 24, 2023 •
edited
Loading

Codecov Report

Merging #2622 (ca78d31) into master (6a51ce5) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head ca78d31 differs from pull request most recent head 1809e15. Consider uploading reports for the commit 1809e15 to get more accurate results

@@            Coverage Diff             @@
##           master    #2622      +/-   ##
==========================================
- Coverage   29.22%   29.21%   -0.02%     
==========================================
  Files         416      416              
  Lines       31318    31328      +10     
==========================================
- Hits         9154     9152       -2     
- Misses      21365    21376      +11     
- Partials      799      800       +1
Files Coverage Δ
pkg/innerring/rpc.go 0.00% <0.00%> (ø)
cmd/neofs-node/config.go 0.00% <0.00%> (ø)
pkg/innerring/innerring.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@carpawell carpawell mentioned this pull request Oct 24, 2023
Merged
@smallhive smallhive force-pushed the sdk-memory-optimizations branch 2 times, most recently from e7784fa to 1379ce2 Compare October 30, 2023 10:14
@smallhive
Copy link
Contributor Author

rebased

@carpawell
Copy link
Member

rebased

😏

@smallhive
go.mod: Memory optimized version of SDK
ca9c8e9 
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive
innerring: Pre-allocated buffer for SDK client
4fefe67 
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive
cache: Pre-allocated buffer for SDK client
1809e15 
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive
Copy link
Contributor Author

Well, one more attempt

@roman-khimov roman-khimov merged commit 9423030 into master Nov 3, 2023
9 checks passed
@roman-khimov roman-khimov deleted the sdk-memory-optimizations branch November 3, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carpawell carpawell carpawell approved these changes

@roman-khimov roman-khimov roman-khimov approved these changes

@cthulhu-rider cthulhu-rider Awaiting requested review from cthulhu-rider cthulhu-rider is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smallhive @carpawell @roman-khimov